Skip to content

Upstream merge 2023 02 13#808

Merged
dkostic merged 11 commits intoaws:mainfrom
dkostic:upstream-merge-2023-02-13
Feb 15, 2023
Merged

Upstream merge 2023 02 13#808
dkostic merged 11 commits intoaws:mainfrom
dkostic:upstream-merge-2023-02-13

Conversation

@dkostic
Copy link
Copy Markdown
Contributor

@dkostic dkostic commented Feb 14, 2023

Issues:

N/A

Description of changes:

Describe AWS-LC’s current behavior and how your code changes that behavior. If there are no issues this pr is resolving,
explain why this change is necessary.

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and
the ISC license.

@dkostic dkostic force-pushed the upstream-merge-2023-02-13 branch 3 times, most recently from c326fc0 to dab618b Compare February 15, 2023 02:38
@dkostic dkostic force-pushed the upstream-merge-2023-02-13 branch from dab618b to 15d051e Compare February 15, 2023 16:38
andrewhop
andrewhop previously approved these changes Feb 15, 2023
Our NCONF_get_section doesn't silently handle NULLs, so add a check to
the caller.

Change-Id: I133d10c7a5dec22469a80b78cb45f479f128ada2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56106
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit 871704b873e356b9eee354b70bf0f11cf234a1f7)
Functions typically just push their own error codes.

Change-Id: Iac83bfdf56ce436bd3a9b8af5c43ca67fb4b9b15
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56107
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit db38fc55939095dd8082e0a89b0c9d187ac164f6)
Squat fewer unprefixed macros.

Update-Note: CTX_TEST appears to be unused. If affected, switch to using
X509V3_set_ctx_test instead.

Change-Id: I43b86c0b6f147bbca85b8bc6b43602fc4f6697c1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56108
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit 65ad925f51d879f8babd22ecb2ca4dc779b2cc24)
This relaxes two caller requirements:

First, although one needs to initialize X509V3_CTX in two parts, some
callers forget to this. This works some of the time on accident,
because most codepaths read ctx->db. But if one were to read it, it'd
be uninitialized. Since all the entrypoints take a CONF anyway, and
always match them, just implicitly initialize the CONF half of the
X509V3_CTX with the provided one.

Second, allow X509V3_CTX to be NULL. Some codepaths in the library
check for NULL (or don't use it) and some do not. Enough codepaths
don't check that it really cannot be considered to work, but enough
do that a caller could mistakenly pass in NULL and have it mostly
work. I've seen one caller mistakenly do this. Since we have to copy
the X509V3_CTX for the first relaxation anyway, allow it to be NULL
and fill in an empty one when omitted.

Update-Note: If using different CONFs in the X509V3_CTX and the function
parameter, the function parameter is now always used. No callers do
this, and it's somewhat arbitrary which is used. (The generic code
always uses the one in ctx. The @section syntax uses the parameter. Then
the per-extension callbacks use the ctx.)

Change-Id: I9fc15a581ea375ea06c4b082dcf0d6360be8144f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56109
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit 114fa727b7281bf532d037036356359619c730be)
This is purely cosmetic, but makes it consistent with the DW, etc.,
lines in the block above. The SEH unwind code support will emit a mix of
DW and DB directives and this makes them look more consistent.

Bug: 259
Change-Id: Ia16166ab2495aa813d6076d55af5b62511933c28
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56125
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit aa18fe24cdbe4d0b353cd3d82a6a5017af88ccb1)
This reimplements policy handling using a similar DAG structure as in
https://chromium-review.googlesource.com/c/chromium/src/+/4111415. The
main difference is that, being C, we don't have std::set or std::map
easily available. But the algorithm can be implemented purely with
sorted lists, while remaining subquadratic.

This implementation relies on two assumptions:

1. We do not return the policy tree. This was removed in
   https://boringssl-review.googlesource.com/c/boringssl/+/53327

2. We do not return the final set of certificate policies. I.e.,
   certificate policy checking is only used for evaluating policy
   constraints and X509_V_FLAG_EXPLICIT_POLICY.

The second assumption is not very important. It mostly simplifies
has_explicit_policy slightly.

In addition, this new implementation removes the per-certificate policy
cache. Instead, we just process the policy extensions anew on
certificate verification. This avoids a mess of threading complexity,
including a race condition in the old logic. See
https://boringssl-review.googlesource.com/c/boringssl/+/55762 for a
description of the race condition.

Change-Id: Ifba9037588ecff5eb6ed3c34c8bd7611f60013a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56036
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
(cherry picked from commit 029d0e77fb64625469cc02c8df26767c72081dfd)
This implements similar directives as MASM, so we do not need to build
all the structures by hand. It does not provide any help to abstract
between Win64 and SysV differences, however.

This is pulled together from some old draft CLs I had, one of which
actually synthesized CFI directives from SEH, so it should be possible.
I've intentionally omitted that however, as it also brings in questions
about how to handle the calling convention differences (the existing
machinery won't *quite* work). I've uploaded just this for now, so
review can focus on the basic mechanism.

I've also preserved perlasm's weird mixed tabs and spaces indentation
convention for now, though it is a bit tedious.

Bug: 259
Change-Id: Ib3f46a27751a5319b758d12c462c660cf9f3e632
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56126
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
(cherry picked from commit c556ee91ff7fc106baadb41ddaaae951ac9b3ba2)
The ABI test already uses CHECK_ABI_SEH, so this is covered under tests.
As part of the conversion, we no longer rely on the assembler generating
a specific encoding and can just write the code normally.

Bug: 259
Change-Id: I47cbf81073237f2b95971a782848b85d230b6bf6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56127
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
(cherry picked from commit ae1546b6f3bf1ad7eb24b491c914eb202b5547d3)
https://boringssl-review.googlesource.com/c/boringssl/+/56109 tried to
simplify the X509V3_CTX story by automatically handling the second half
of initialization, but it turns out not all callers specify both values.

Instead, align with OpenSSL 3.0's behavior. Now X509V3_set_ctx
implicitly zeros the other fields, so it is the only mandatory init
function. This does mean callers which call X509V3_set_nconf before
X509V3_set_ctx will break, but that's true in OpenSSL 3.0 too.

I've retained the allowance for ctx being NULL, because whether
functions tolerate that or not is still a bit inconsistent. Also added
some TODOs about how strange this behavior is, but it's probably not
worth spending much more time on this code.

Change-Id: Ia04cf11eb5158374ca186795b7e579575e80666f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56265
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
(cherry picked from commit 1c9d18307bdf75bee7f3ec7f7d2e421a734831f1)
Some profiling systems cannot unwind with CFI and benefit from having a
frame pointer. Since this code doesn't have enough register pressure to
actually need to use rbp as a general register, this change tweaks
things so that a frame pointer is preserved.

As this would invalidate the SEH handler, just replace it with proper
unwind codes, which are more profiler-friendly and supportable by our
unwind tests. Some notes on this:

- We don't currently support the automatic calling convention conversion
  with unwind codes, but this file already puts all arguments in
  registers, so I just renamed the arguments and put the last two
  arguments in RDI and RSI. Those I stashed into the parameter stack
  area because it's free storage.

- It is tedious to write the same directives in both CFI and SEH. We
  really could do with an abstraction. Although since most of our
  functions need a Windows variation anyway.

- I restored the original file's use of PUSH to save the registers.
  This matches what Clang likes to output anyway, and push is probably
  smaller than the corresponding move with offset. (And it reduces how
  much thinking about offsets I need to do.)

- Although it's an extra instruction, I restored the original file's
  separate fixed stack allocation and alloca for the sake of clarity.

- The epilog is constrained by Windows being extremely picky about
  epilogs. (Windows doesn't annotate epilogs and instead simulates
  forward.) I think other options are possible, but using LEA with an
  offset to realign the stack for the POPs both matches the examples in
  Windows and what Clang seems to like to output. The original file used
  MOV with offset, but it seems to be related to the funny SEH handler.

- The offsets in SEH directives may be surprising to someone used to CFI
  directives or a SysV RBP frame pointer. All three use slightly
  different baselines:

  CFI's canonical frame address (CFA) is RSP just before a CALL (so
  before the saved RIP in stack order). It is 16-byte aligned by ABI.

  A SysV RBP frame pointer is 16 bytes after that, after a saved RIP and
  saved RBP. It is also 16-byte aligned.

  Windows' baseline is the top of the fixed stack allocation, so
  potentially some bytes after that (all pushreg and allocstack
  directives). This too is required to be 16-byte aligned.

  Windows, however, doesn't require the frame register actually contain
  the fixed stack allocation. You can specify an offset from the value
  in the register to the actual top. But all the offsets in savereg,
  etc., directives use this baseline.

Performance difference is within measurement noise.

This does not create a stack frame for internal functions so
frame-pointer unwinding may miss a function or two, but the broad
attribution will be correct.

Change originally by Clemens Fruhwirth. Then reworked from Adam
Langley's https://boringssl-review.googlesource.com/c/boringssl/+/55945
by me to work on Windows and fix up some issues with the RBP setup.

Bug: b/33072965, 259
Change-Id: I52302635a8ad3d9272404feac125e2a4a4a5d14c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56128
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit 0d5b6086143d19f86cc5d01b8944a1c13f99be24)
There was a typo and inhibitPolicyMapping updated the wrong value. With
this fixed, we pass the PKITS tests (as imported into Chromium).

NOTE: Imported the fix only, skipped the new tests because they depend
on some previously skipped commits. Added action items to the backlog.

Change-Id: I3b80eb56561ae5ae88023fa639d697a9f1757b21
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56205
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit ba68ca070ca939bc2d6b8f07bd64909bd90b25a5)
@dkostic dkostic merged commit d7e57de into aws:main Feb 15, 2023
R3hankhan123 pushed a commit to R3hankhan123/aws-lc that referenced this pull request Aug 29, 2025
* Disallow external bindgen when AWS_LC_SYS_EXTERNAL_BINDGEN=0

* Fix s390x-unknown-linux-gnu CI test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants