Merge branch 'main' into penpal-dos-bounds

Author: prasden

.github/workflows/security-review.yml

@@ -0,0 +1,84 @@
+name: security-review
+
+on:
+  pull_request_target:
+    branches: ["*"]
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref_name }}
+  cancel-in-progress: true
+permissions:
+  contents: read
+
+jobs:
+  authorize:
+    runs-on: ubuntu-latest
+    outputs:
+      approval-env: ${{ steps.authorization.outputs.approval-env }}
+    steps:
+      - uses: actions/checkout@v4
+      - name: Check authorization
+        id: authorization
+        uses: ./.github/actions/check-authorization
+
+  execute:
+    needs: authorize
+    runs-on: ubuntu-latest
+    environment: ${{ needs.authorize.outputs.approval-env }}
+    permissions:
+      id-token: write
+      contents: read
+      statuses: write
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+
+      - name: Get AWS credentials
+        uses: aws-actions/configure-aws-credentials@v4
+        with:
+          role-to-assume: arn:aws:iam::547182295936:role/SecurityReview-GitHubOIDCRole #TODO: migrate to production account
+          role-session-name: ${{ github.run_id }}-${{ github.run_attempt }}
+          aws-region: us-west-2
+
+      - name: Start CodeBuild and wait for completion
+        id: codebuild
+        shell: bash
+        env:
+          PROJECT_NAME: SecurityReview-${{ github.event.repository.name }}
+          SOURCE_VERSION: "pr/${{ github.event.pull_request.number }}"
+          PR_NUMBER: ${{ github.event.pull_request.number }}
+        run: |
+          # Start the build
+          BUILD_ID=$(aws codebuild start-build \
+            --project-name "${PROJECT_NAME}" \
+            --source-version "${SOURCE_VERSION}" \
+            --environment-variables-override "name=PR_NUMBER,value=${PR_NUMBER},type=PLAINTEXT" \
+            --query 'build.id' --output text)
+
+          # Wait for completion
+          while STATUS=$(aws codebuild batch-get-builds --ids "${BUILD_ID}" --query 'builds[0].buildStatus' --output text); [[ "$STATUS" == "IN_PROGRESS" ]]; do
+            sleep 30
+          done
+
+          if [[ "$STATUS" != "SUCCEEDED" ]]; then
+            echo "blocking=skip" >> "$GITHUB_OUTPUT"
+            exit 1
+          fi
+
+          REVIEW_STATUS=$(aws codebuild batch-get-builds --ids "${BUILD_ID}" --query 'builds[0].exportedEnvironmentVariables[?name==`REVIEW_STATUS`].value' --output text)
+          echo "blocking=$([[ "$REVIEW_STATUS" == "FAIL" ]] && echo true || echo false)" >> "$GITHUB_OUTPUT"
+
+      - name: Update commit status
+        if: always() && steps.codebuild.outputs.blocking != 'skip'
+        shell: bash
+        env:
+          GH_TOKEN: ${{ github.token }}
+          STATUS_URL: ${{ github.api_url }}/repos/${{ github.repository }}/statuses/${{ github.event.pull_request.head.sha }}
+          REPORT_URL: https://d28bfvmis1skm5.cloudfront.net/${{ github.event.repository.name }}/pr-${{ github.event.pull_request.number }}/${{ github.event.pull_request.head.sha }}.html
+          BLOCKING: ${{ steps.codebuild.outputs.blocking }}
+        run: |
+          STATE=$([[ "$BLOCKING" == "true" ]] && echo "failure" || echo "success")
+          curl -sS -X POST \
+            -H "Authorization: token ${GH_TOKEN}" \
+            "${STATUS_URL}" \
+            -d "{\"state\":\"${STATE}\",\"context\":\"security-review / report\",\"target_url\":\"${REPORT_URL}\"}"

.github/workflows/zig.yml

@@ -7,6 +7,10 @@ on:
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref_name }}
   cancel-in-progress: true
+
+permissions:
+  contents: read
+
 jobs:
   zig:
     if: github.repository_owner == 'aws'

crypto/asn1/a_mbstr.c

@@ -33,6 +33,10 @@ OPENSSL_DECLARE_ERROR_REASON(ASN1, INVALID_UTF8STRING)
 int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in,
                         ossl_ssize_t len, int inform, unsigned long mask,
                         ossl_ssize_t minsize, ossl_ssize_t maxsize) {
+  if (len < -1) {
+    OPENSSL_PUT_ERROR(ASN1, ASN1_R_ILLEGAL_FORMAT);
+    return -1;
+  }
   if (len == -1) {
     len = strlen((const char *)in);
   }

crypto/asn1/asn1_test.cc

@@ -1711,6 +1711,25 @@ TEST(ASN1Test, MBString) {
     ERR_clear_error();
     EXPECT_EQ(nullptr, str);
   }
+
+  // |len| values below -1 must be rejected; only -1 is special-cased to mean
+  // "call strlen on |in|". Any other negative value would otherwise be cast
+  // to a huge |size_t| by |CBS_init|.
+  static const uint8_t kDummy[] = {'a'};
+  ASN1_STRING *str = nullptr;
+  EXPECT_EQ(-1, ASN1_mbstring_ncopy(&str, kDummy, -2, MBSTRING_UTF8,
+                                    B_ASN1_UTF8STRING, /*minsize=*/0,
+                                    /*maxsize=*/0));
+  EXPECT_EQ(nullptr, str);
+  ERR_clear_error();
+
+  // |len == -1| treats |in| as NUL-terminated and should succeed.
+  ASN1_STRING *str_from_strlen = nullptr;
+  EXPECT_GE(ASN1_mbstring_ncopy(&str_from_strlen, (const uint8_t *)"a", -1,
+                                MBSTRING_UTF8, B_ASN1_UTF8STRING,
+                                /*minsize=*/0, /*maxsize=*/0),
+            0);
+  ASN1_STRING_free(str_from_strlen);
 }
 
 TEST(ASN1Test, StringByNID) {

crypto/bio/bio_addr.c

@@ -110,12 +110,12 @@ int BIO_ADDR_rawmake(BIO_ADDR *ap, int family, const void *where,
     GUARD_PTR(where);
     // wherelen is expected to be the length of the path string
     // not including the terminating NUL.
-    if (wherelen + 1 > sizeof(ap->s_un.sun_path)) {
+    if (wherelen >= sizeof(ap->s_un.sun_path)) {
       return 0;
     }
     OPENSSL_cleanse(&ap->s_un, sizeof(ap->s_un));
     ap->s_un.sun_family = family;
-    OPENSSL_strlcpy(ap->s_un.sun_path, where, wherelen);
+    OPENSSL_memcpy(ap->s_un.sun_path, where, wherelen);
     return 1;
   }
 #endif
@@ -151,7 +151,10 @@ int BIO_ADDR_rawaddress(const BIO_ADDR *ap, void *p, size_t *l) {
     return 0;
   }
   if (l != NULL) {
-    *l = len;
+    // AF_UNIX includes the trailing NUL written below so |*l| matches the
+    // bytes written to |p| and reflects sockaddr_un's NUL-terminated path.
+    // OpenSSL does not write this NUL; aws-lc intentionally does.
+    *l = (ap->sa.sa_family == AF_UNIX) ? len + 1 : len;
   }
 
   if (p != NULL) {

crypto/bio/bio_socket_test.cc

@@ -130,7 +130,7 @@ struct SockaddrStorage {
         if (1 !=
             BIO_ADDR_rawmake(
                 bap, AF_UNIX, &sock->sun_path,
-                OPENSSL_strnlen(sock->sun_path, sizeof(sock->sun_path)) + 1,
+                OPENSSL_strnlen(sock->sun_path, sizeof(sock->sun_path)),
                 0)) {
           BIO_ADDR_free(bap);
           bap = nullptr;
@@ -963,6 +963,24 @@ TEST_F(BIOAddrTest, CopySpecifiedAddressUnix) {
   EXPECT_EQ(BIO_ADDR_copy(addr1, addr2), 1);
   EXPECT_EQ(BIO_ADDR_cmp(addr1, addr2), 0);
 }
+
+// |BIO_ADDR_rawaddress| writes a NUL terminator after the AF_UNIX path. The
+// reported |*l| must include that NUL so that callers using the standard
+// probe-then-allocate-exactly-|*l| pattern allocate a buffer large enough to
+// hold both the path and the terminator.
+TEST_F(BIOAddrTest, RawAddressUnixLengthIncludesNul) {
+  const char *path = "/tmp/test.sock";
+  const size_t path_len = strlen(path);
+  ASSERT_EQ(BIO_ADDR_rawmake(addr1, AF_UNIX, path, path_len, 0), 1);
+
+  size_t len = 0;
+  ASSERT_EQ(BIO_ADDR_rawaddress(addr1, nullptr, &len), 1);
+  EXPECT_EQ(len, path_len + 1);
+
+  char buf[sizeof(sockaddr_un::sun_path)] = {};
+  ASSERT_EQ(BIO_ADDR_rawaddress(addr1, buf, &len), 1);
+  EXPECT_STREQ(buf, path);
+}
 #endif
 
 // Tests for BIO_ADDR_dup

crypto/cipher_extra/cipher_test.cc

@@ -20,6 +20,7 @@
 #include <openssl/sha.h>
 #include <openssl/span.h>
 
+#include "../fipsmodule/cipher/internal.h"
 #include "../internal.h"
 #include "../test/file_test.h"
 #include "../test/test_util.h"
@@ -1406,6 +1407,106 @@ TEST(CipherTest, Empty_EVP_CIPHER_CTX_V1187459157) {
   CHECK_ERROR(EVP_DecryptFinal(ctx.get(), out_vec.data(), &out_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
 }
 
+// Mock cipher whose |EVP_CTRL_INIT| handler always fails. It's needed to
+// exercise the error paths in |EVP_CipherInit_ex| and |EVP_CIPHER_CTX_copy|.
+// In addition, the mock needs |ctx_size| != 0, otherwise |cipher_data| is
+// not allocated before the ctrl callback runs. This is needed below for
+// InitErrorPathReleasesCipherData, to test |cipher_data| is free'd.
+static int mock_failing_init(EVP_CIPHER_CTX *, const uint8_t *, const uint8_t *,
+                             int) {
+  return 1;
+}
+
+static int mock_failing_cipher(EVP_CIPHER_CTX *, uint8_t *, const uint8_t *,
+                               size_t) {
+  return 1;
+}
+
+static int mock_failing_ctrl(EVP_CIPHER_CTX *, int type, int, void *) {
+  if (type == EVP_CTRL_INIT) {
+    return 0;
+  }
+  return -1;
+}
+
+static int mock_failing_copy_ctrl(EVP_CIPHER_CTX *, int type, int, void *) {
+  if (type == EVP_CTRL_INIT) {
+    return 1;
+  }
+  if (type == EVP_CTRL_COPY) {
+    return 0;
+  }
+  return -1;
+}
+
+static const EVP_CIPHER kFailingInitCipher = {
+  NID_undef,
+  1,
+  16,
+  0,
+  128,
+  EVP_CIPH_STREAM_CIPHER | EVP_CIPH_CTRL_INIT,
+  mock_failing_init,
+  mock_failing_cipher,
+  nullptr,
+  mock_failing_ctrl,
+};
+
+static const EVP_CIPHER kFailingCopyCipher = {
+  NID_undef,
+  1,
+  16,
+  0,
+  128,
+  EVP_CIPH_STREAM_CIPHER | EVP_CIPH_CTRL_INIT | EVP_CIPH_CUSTOM_COPY,
+  mock_failing_init,
+  mock_failing_cipher,
+  nullptr,
+  mock_failing_copy_ctrl,
+};
+
+// On |EVP_CipherInit_ex| error path at |EVP_CTRL_INIT|, the context must not be
+// left with an orphaned |cipher_data|. Otherwise a subsequent
+// |EVP_CipherInit_ex| call on the same context would overwrite and leak it. Not
+// exactly how one would probably use this API, but who knows.
+TEST(CipherTest, InitErrorPathReleasesCipherData) {
+  bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new());
+  ASSERT_TRUE(ctx);
+
+  std::vector<uint8_t> key(16, 0);
+  EXPECT_FALSE(EVP_CipherInit_ex(ctx.get(), &kFailingInitCipher, nullptr,
+                                 key.data(), nullptr, 1));
+  EXPECT_EQ(ctx->cipher, nullptr);
+  EXPECT_EQ(ctx->cipher_data, nullptr);
+
+  // A follow-up init with a real cipher must still work and must not depend on
+  // leaked state from the failed attempt.
+  EXPECT_TRUE(EVP_CipherInit_ex(ctx.get(), EVP_aes_128_ecb(), nullptr,
+                                key.data(), nullptr, 1));
+}
+
+// |EVP_CIPHER_CTX_copy| shares the same shape of error path for
+// |EVP_CTRL_COPY| failures as |EVP_CTRL_INIT| above. The destination's
+// |cipher_data| must not be orphaned when the custom copy callback fails.
+TEST(CipherTest, CopyErrorPathReleasesCipherData) {
+  bssl::UniquePtr<EVP_CIPHER_CTX> src(EVP_CIPHER_CTX_new());
+  ASSERT_TRUE(src);
+  std::vector<uint8_t> key(16, 0);
+  ASSERT_TRUE(EVP_CipherInit_ex(src.get(), &kFailingCopyCipher, nullptr,
+                                key.data(), nullptr, 1));
+
+  bssl::UniquePtr<EVP_CIPHER_CTX> dst(EVP_CIPHER_CTX_new());
+  ASSERT_TRUE(dst);
+  EXPECT_FALSE(EVP_CIPHER_CTX_copy(dst.get(), src.get()));
+  EXPECT_EQ(dst->cipher, nullptr);
+  EXPECT_EQ(dst->cipher_data, nullptr);
+
+  // A follow-up init with a real cipher on |dst| must still work and must not
+  // depend on leaked state from the failed copy.
+  EXPECT_TRUE(EVP_CipherInit_ex(dst.get(), EVP_aes_128_ecb(), nullptr,
+                                key.data(), nullptr, 1));
+}
+
 struct CipherInfo {
   const char *name;
   const EVP_CIPHER *(*func)(void);

crypto/evp_extra/evp_asn1.c

@@ -172,6 +172,12 @@ EVP_PKEY *EVP_parse_private_key(CBS *cbs) {
     has_pub = 1;
   }
 
+  // Reject trailing data within the SEQUENCE.
+  if (CBS_len(&pkcs8) != 0) {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
+    return NULL;
+  }
+
   // Set up an |EVP_PKEY| of the appropriate type.
   EVP_PKEY *ret = EVP_PKEY_new();
   if (ret == NULL) {

crypto/evp_extra/evp_extra_test.cc

@@ -1683,6 +1683,30 @@ TEST(EVPExtraTest, Ed25519) {
   ERR_clear_error();
 }
 
+// EVP_parse_private_key must reject trailing data inside the PrivateKeyInfo
+// SEQUENCE.
+TEST(EVPExtraTest, ParsePrivateKeyRejectsTrailingData) {
+  // Ed25519 PKCS#8 (the kPrivateKeyPKCS8 vector from EVPExtraTest.Ed25519)
+  // with two extra bytes (ASN.1 NULL: 05 00) appended inside the outer
+  // SEQUENCE. The SEQUENCE length byte is bumped 0x2e -> 0x30 to cover
+  // the extra bytes, so the outer TLV is itself well-formed.
+  static const uint8_t kDERWithTrailing[] = {
+      0x30, 0x30, 0x02, 0x01, 0x00, 0x30, 0x05, 0x06, 0x03, 0x2b, 0x65, 0x70,
+      0x04, 0x22, 0x04, 0x20, 0x9d, 0x61, 0xb1, 0x9d, 0xef, 0xfd, 0x5a, 0x60,
+      0xba, 0x84, 0x4a, 0xf4, 0x92, 0xec, 0x2c, 0xc4, 0x44, 0x49, 0xc5, 0x69,
+      0x7b, 0x32, 0x69, 0x19, 0x70, 0x3b, 0xac, 0x03, 0x1c, 0xae, 0x7f, 0x60,
+      0x05, 0x00,
+  };
+
+  CBS cbs;
+  CBS_init(&cbs, kDERWithTrailing, sizeof(kDERWithTrailing));
+  ERR_clear_error();
+  bssl::UniquePtr<EVP_PKEY> parsed(EVP_parse_private_key(&cbs));
+  EXPECT_FALSE(parsed);
+  EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_EVP, EVP_R_DECODE_ERROR));
+  ERR_clear_error();
+}
+
 // EVP_PKEY_get_private_seed returns an error for key types that don't
 // provide a get_priv_seed method. It must also reject NULL |key| regardless of
 // key type.

crypto/evp_extra/p_kem_asn1.c

@@ -86,7 +86,14 @@ static int kem_get_pub_raw(const EVP_PKEY *pkey, uint8_t *out,
   return 1;
 }
 
+// kem_cmp_parameters returns 1 if |a| and |b| hold populated KEM keys with
+// the same KEM NID, 0 if their NIDs differ, or -2 if either operand is
+// missing its key or parameters. The tri-state return aligns with the
+// |EVP_PKEY_cmp| convention (1 = equal, 0 = not equal, negative = error).
 static int kem_cmp_parameters(const EVP_PKEY *a, const EVP_PKEY *b) {
+  if (a == NULL || b == NULL) {
+    return -2;
+  }
   const KEM_KEY *a_key = a->pkey.kem_key;
   const KEM_KEY *b_key = b->pkey.kem_key;
   if (a_key == NULL || b_key == NULL) {