Ensure no trailing data for PKCS8 EVP_parse_private_key

[torben-hansen]

Description of changes:

Similar to EVP_parse_public_key(), don't allow trailing data. This matches what EVP_parse_public_key() does.

Testing:

Cover this with a test case.

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.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.29%. Comparing base (25a859c) to head (5003d9c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3242      +/-   ##
==========================================
+ Coverage   78.12%   78.29%   +0.16%     
==========================================
  Files         689      689              
  Lines      123219   123229      +10     
  Branches    17138    17143       +5     
==========================================
+ Hits        96270    96485     +215     
+ Misses      26038    25831     -207     
- Partials      911      913       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[justsmth]

Could we also test a V2?

// EVP_parse_private_key must also reject trailing data inside the SEQUENCE
// after the optional [1] publicKey of an RFC 5958 v2 OneAsymmetricKey. This
// pins the invariant that the trailing-data check fires after the optional
// fields are consumed.
TEST(EVPExtraTest, ParsePrivateKeyV2RejectsTrailingData) {
  // Base vector is the v2 OneAsymmetricKey from
  // EVPExtraTest.Ed25519PKCS8v2WithAttributes (empty [0] attributes and a
  // [1] publicKey), with two extra bytes (ASN.1 NULL: 05 00) appended inside
  // the outer SEQUENCE. The outer SEQUENCE length byte is bumped
  // 0x53 -> 0x55 to cover the extra bytes, so the outer TLV is itself
  // well-formed.
  static const uint8_t kDERWithTrailing[] = {
    0x30, 0x55,                                     // SEQUENCE, len 0x55
      0x02, 0x01, 0x01,                             // version = 1 (v2)
      0x30, 0x05, 0x06, 0x03, 0x2b, 0x65, 0x70,     // AlgId { OID 1.3.101.112 }
      0x04, 0x22,                                   // OCTET STRING, len 0x22
        0x04, 0x20,                                 //   OCTET STRING, len 32
        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,
      0xa0, 0x00,                                   // [0] attributes = { }
      0x81, 0x21, 0x00,                             // [1] publicKey BIT STRING
        0xd7, 0x5a, 0x98, 0x01, 0x82, 0xb1, 0x0a, 0xb7,
        0xd5, 0x4b, 0xfe, 0xd3, 0xc9, 0x64, 0x07, 0x3a,
        0x0e, 0xe1, 0x72, 0xf3, 0xda, 0xa6, 0x23, 0x25,
        0xaf, 0x02, 0x1a, 0x68, 0xf7, 0x07, 0x51, 0x1a,
      0x05, 0x00,                                   // trailing ASN.1 NULL
  };

  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();
}

[torben-hansen]

It's basically redundant because the strict no trailing data is not an invariant that depends on the version. The sequence exist in both versions.