Skip to content

Reject imported EC and RSA keys with trailing bytes #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
harrshita123 wants to merge 7 commits into
base: master
Choose a base branch
from
+139 −12
Open

Reject imported EC and RSA keys with trailing bytes #251

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9566c6f
Reject imported EC and RSA keys with trailing bytes
harrshita123 Feb 7, 2026
0a2fe1d
Add regression tests for issue 60 (trailing bytes in key imports)
harrshita123 Apr 26, 2026
b402d66
Finalize regression tests for issue 60: hardcoded keys and browser co…
harrshita123 Apr 27, 2026
4b903e1
Update lib/src/testing/regression/issue_60_trailing_bytes.dart
harrshita123 Apr 29, 2026
b25d912
Update lib/src/testing/regression/issue_60_trailing_bytes.dart
harrshita123 Apr 29, 2026
2a3136d
Fix trailing bytes regression fixture
harrshita123 May 8, 2026
f9c752d
Minimize trailing bytes fixture diff
harrshita123 May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions lib/src/impl_ffi/impl_ffi.ec_common.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,14 @@ void _validateEllipticCurveKey(_EvpPKey key, EllipticCurve curve) {

_EvpPKey _importPkcs8EcPrivateKey(List<int> keyData, EllipticCurve curve) {
return _Scope.sync((scope) {
final k = ssl.EVP_parse_private_key(scope.createCBS(keyData));
_checkData(k.address != 0, fallback: 'unable to parse key');
final cbs = scope.createCBS(keyData);
final k = ssl.EVP_parse_private_key(cbs);

_checkData(
k.address != 0 && cbs.ref.len == 0,
Comment thread
harrshita123 marked this conversation as resolved.
fallback: 'unable to parse key',
);

final key = _EvpPKey.wrap(k);
_validateEllipticCurveKey(key, curve);
return key;
Expand All @@ -99,13 +105,15 @@ _EvpPKey _importPkcs8EcPrivateKey(List<int> keyData, EllipticCurve curve) {

_EvpPKey _importSpkiEcPublicKey(List<int> keyData, EllipticCurve curve) {
return _Scope.sync((scope) {
// TODO: When calling EVP_parse_public_key it might wise to check that CBS_len(cbs) == 0 is true afterwards
// otherwise it might be that all of the contents of the key was not consumed and we should throw
// a FormatException. Notice that this the case for private/public keys, and RSA keys.
final k = ssl.EVP_parse_public_key(scope.createCBS(keyData));
_checkData(k.address != 0, fallback: 'unable to parse key');
final key = _EvpPKey.wrap(k);
final cbs = scope.createCBS(keyData);
final k = ssl.EVP_parse_public_key(cbs);

_checkData(
k.address != 0 && cbs.ref.len == 0,
fallback: 'unable to parse key',
);

final key = _EvpPKey.wrap(k);
_validateEllipticCurveKey(key, curve);

return key;
Expand Down
20 changes: 16 additions & 4 deletions lib/src/impl_ffi/impl_ffi.rsa_common.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ part of 'impl_ffi.dart';

_EvpPKey _importPkcs8RsaPrivateKey(List<int> keyData) {
return _Scope.sync((scope) {
final k = ssl.EVP_parse_private_key(scope.createCBS(keyData));
_checkData(k.address != 0, fallback: 'unable to parse key');
final cbs = scope.createCBS(keyData);
final k = ssl.EVP_parse_private_key(cbs);

_checkData(
k.address != 0 && cbs.ref.len == 0,
fallback: 'unable to parse key',
);

final key = _EvpPKey.wrap(k);

_checkData(
Expand All @@ -37,8 +43,14 @@ _EvpPKey _importPkcs8RsaPrivateKey(List<int> keyData) {

_EvpPKey _importSpkiRsaPublicKey(List<int> keyData) {
return _Scope.sync((scope) {
final k = ssl.EVP_parse_public_key(scope.createCBS(keyData));
_checkData(k.address != 0, fallback: 'unable to parse key');
final cbs = scope.createCBS(keyData);
final k = ssl.EVP_parse_public_key(cbs);

_checkData(
k.address != 0 && cbs.ref.len == 0,
fallback: 'unable to parse key',
);

final key = _EvpPKey.wrap(k);

_checkData(
Expand Down
84 changes: 84 additions & 0 deletions lib/src/testing/regression/issue_60_trailing_bytes.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright 2026 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:typed_data';
import 'package:webcrypto/webcrypto.dart';
import '../utils/utils.dart';

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

/// 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

@jonasfj jonasfj Apr 27, 2026

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 •
edited
Loading

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.


test('Ecdsa: importPkcs8Key with trailing bytes throws FormatException', () async {
final kp = await EcdsaPrivateKey.generateKey(EllipticCurve.p256);
final validKey = await kp.privateKey.exportPkcs8Key();
final invalidKey = Uint8List.fromList([...validKey, 0]);

bool threw = false;
try {
await EcdsaPrivateKey.importPkcs8Key(invalidKey, EllipticCurve.p256);
} on FormatException {
threw = true;
}
check(threw, 'Expected FormatException when importing EC private key with trailing bytes');
});

test('Ecdsa: importSpkiKey with trailing bytes throws FormatException', () async {
final kp = await EcdsaPrivateKey.generateKey(EllipticCurve.p256);
final validKey = await kp.publicKey.exportSpkiKey();
final invalidKey = Uint8List.fromList([...validKey, 0]);

bool threw = false;
try {
await EcdsaPublicKey.importSpkiKey(invalidKey, EllipticCurve.p256);
} on FormatException {
threw = true;
}
check(threw, 'Expected FormatException when importing EC public key with trailing bytes');
});

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

@jonasfj jonasfj Apr 27, 2026

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

@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.

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.

final validKey = await kp.privateKey.exportPkcs8Key();
final invalidKey = Uint8List.fromList([...validKey, 0]);

bool threw = false;
try {
await RsaPssPrivateKey.importPkcs8Key(invalidKey, Hash.sha256);
} on FormatException {
threw = true;
}
check(threw, 'Expected FormatException when importing RSA private key with trailing bytes');
});

test('RsaPss: importSpkiKey with trailing bytes throws FormatException', () async {
final kp = await RsaPssPrivateKey.generateKey(2048, BigInt.from(65537), Hash.sha256);
final validKey = await kp.publicKey.exportSpkiKey();
final invalidKey = Uint8List.fromList([...validKey, 0]);

bool threw = false;
try {
await RsaPssPublicKey.importSpkiKey(invalidKey, Hash.sha256);
} on FormatException {
threw = true;
}
check(threw, 'Expected FormatException when importing RSA public key with trailing bytes');
});

return tests;
}
2 changes: 2 additions & 0 deletions lib/src/testing/testing.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import 'webcrypto/rsassapkcs1v15.dart' as rsassapkcs1v15;
// Other test files, that don't use TestRunner
import 'webcrypto/random.dart' as random;
import 'webcrypto/digest.dart' as digest;
import 'regression/issue_60_trailing_bytes.dart' as issue_60_trailing_bytes;

/// Test runners from all test files except `digest.dart` and
/// `random.dart`, which do not use [TestRunner].
Expand Down Expand Up @@ -58,6 +59,7 @@ void runAllTests(
for (final r in _testRunners) ...r.tests(),
...random.tests(),
...digest.tests(),
...issue_60_trailing_bytes.tests(),
];

for (final (:name, :test) in allTests) {
Expand Down
Loading