Skip to content

Reject imported EC and RSA keys with trailing bytes#251

Open
harrshita123 wants to merge 7 commits into
google:masterfrom
harrshita123:fix-cbs-empty-after-import
Open

Reject imported EC and RSA keys with trailing bytes#251
harrshita123 wants to merge 7 commits into
google:masterfrom
harrshita123:fix-cbs-empty-after-import

Conversation

@harrshita123
Copy link
Copy Markdown
Contributor

This change ensures that all bytes supplied during key import are consumed by BoringSSL.

EC and RSA private/public key imports now verify that the CBS is empty after parsing, rejecting inputs with trailing bytes. This resolves the TODO in EC public key import and aligns behavior with WebCrypto implementations.

Tests for malformed key bytes are not included here, as generating invalid key material is currently blocked by #55.

Related to #60.

Comment thread lib/src/impl_ffi/impl_ffi.ec_common.dart
});

test('RsaPss: importPkcs8Key with trailing bytes throws FormatException', () async {
final kp = await RsaPssPrivateKey.generateKey(2048, BigInt.from(65537), Hash.sha256);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also hardcode these keys, to make tests faster and more consistent.

Maybe it might also be worth testing that before we append zero, we can't import, and when we've appended zero we can't import. This way we're sure it's not because we have hardcoded an invalid :D

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I replaced generated keys with hardcoded EC/RSA PKCS8 and SPKI keys to make the tests faster
and deterministic. Each test now first imports the valid key successfully, then appends a trailing
byte and verifies that import is rejected.

Comment on lines +21 to +25
/// Tests for issue #60, exported for use in `../testing.dart`.
List<({String name, Future<void> Function() test})> tests() {
final tests = <({String name, Future<void> Function() test})>[];
void test(String name, Future<void> Function() testFn) =>
tests.add((name: name, test: testFn));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add something like this to utils/utils.dart

List<({String name, Future<void> Function() test})> defineTests(
  void Function(void Function(String name, Future<void> Function() testFn) test) tests
) {
  final tests_ = <({String name, Future<void> Function() test})>[];
  void test(String name, Future<void> Function() testFn) =>
      tests_.add((name: name, test: testFn));
  tests(test);
  return tests_;
}

Then this could just be:

void main() => tests.runTests();

final tests = defineTests((test) {
  test('...', () async {
    ...
  });

  ...
});

But we don't have to do that in this PR. I'm happy to land this as is.

Copy link
Copy Markdown
Contributor Author

@harrshita123 harrshita123 Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the requested regression test coverage is added and wired into runAllTests. I also updated the
tests to use hardcoded keys and verify the original valid key imports before checking rejection
with trailing bytes. I left the defineTests helper out for now since that was marked as optional
for this PR.

Comment thread lib/src/testing/regression/issue_60_trailing_bytes.dart Outdated
Comment thread lib/src/testing/regression/issue_60_trailing_bytes.dart Outdated
threw = true;
}
if (!_isWeb) {
check(threw, 'Should throw FormatException for trailing bytes');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens that is different on web?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On web, crypto.subtle.importKey() rejects the malformed PKCS8/SPKI input with trailing bytes, and our JS wrapper maps that browser DataError to FormatException. The FFI backend was previously more permissive because BoringSSL parsed the valid prefix and left the trailing bytes unread. This change makes FFI match the browser behavior by requiring the input to be fully consumed.

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.

3 participants