Skip to content
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.

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

parse_ccs fixes #327

Merged
merged 2 commits into from
Nov 29, 2024
Merged

Conversation

chrysn
Copy link
Collaborator

@chrysn chrysn commented Nov 27, 2024

parse_ccs is very lax right now, and that can cause great confusion: when sending a key in an unexpected format, a panic may happen deep inside p256 / subtle, around public keys being not a good point. (We should catch that panic either way, but this had sent me to the completely wrong track).

This adds a test for whether the parsing function rightfully errs out on stuff it doesn't recognize, and rewrites the parsing so that it does err out.

The current code accepts them but produces wrong public keys, resulting
in highly confusing `assertion `left == right` failed: Public key is not
a good point` errors being raised from subtle/p256_ecdh.
@chrysn
Copy link
Collaborator Author

chrysn commented Nov 27, 2024

Two notes on the function's behavior, apart from the obvious new rejection of "malformed" items:

  • It does accept longer KIDs now, simply because I'd have to go out of my way not to.
  • It does not yet come down hard on invalid-length y coordinates, a) because they're not processed any further, but b) also because I cheated in earlier experiments and put y='' in to accommodate for credential-by-value space constraints. (On the long run, we should reject wrong y values, but maybe we should consider whether we regard y as optional, especially given that at least the credentials we're using now are only used for ECDH where the y doesn't matter).

@chrysn
Copy link
Collaborator Author

chrysn commented Nov 27, 2024

The panicing is already tracked in #93, and there's a note in the code on it around

// While this can actually panic so far, the proper fix is in

@geonnave
Copy link
Collaborator

Looks good, thanks!

@geonnave geonnave merged commit a6b2570 into openwsn-berkeley:main Nov 29, 2024
37 checks passed
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.

2 participants