Skip to content

Reject imported EC and RSA keys with trailing bytes#251

Open
harrshita123 wants to merge 2 commits intogoogle:masterfrom
harrshita123:fix-cbs-empty-after-import
Open

Reject imported EC and RSA keys with trailing bytes#251
harrshita123 wants to merge 2 commits intogoogle: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.

final k = ssl.EVP_parse_private_key(cbs);

_checkData(
k.address != 0 && cbs.ref.len == 0,
Copy link
Copy Markdown
Member

@jonasfj jonasfj Apr 24, 2026

Choose a reason for hiding this comment

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

We need tests that confirm that this what all the browser implementations also do.

I wonder if we should simply write some hand written tests? that don't necessarily use the testrunner setup..

Maybe, we could do something like:

lib/src/testing/regression/issue_<id>_<name>.dart

And have such files do:

import '../utils/test.dart';

void runTests(void Function(String name, Future<void> Function() test) testFn) {
  test('something', () async {
    ...
  });
}

Then we can have lib/src/testing/testing.dart import and call runTests in its runAllTests function.

@HamdaanAliQuatil WDYT?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the idea of an issue-specific regression bucket.

Since runAllTests already aggregates non-TestRunner tests like random.dart and digest.dart, I’d follow that existing pattern and add something like lib/src/testing/regression/issue_60_trailing_bytes.dart exposing tests(), then append those in lib/src/testing/testing.dart.

That said, I wouldn’t want regression/issue_<id>_... to become the default pattern for lots of tests. I’d treat it as a narrow escape hatch for browser-compat regressions and other negative cases that don’t cleanly fit the current framework.

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