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.

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

Jump to bottom

Better NULL input checking? #47

Closed
michelp opened this issue Nov 28, 2022 · 3 comments · Fixed by #50
Closed

Better NULL input checking? #47

michelp opened this issue Nov 28, 2022 · 3 comments · Fixed by #50

Comments

@michelp
Copy link
Owner

michelp commented Nov 28, 2022 •
edited
Loading

Reported by @ioguix in #43

By the way, note that if I give NULL as additional data, it just crash:

pgsodium.crypto_aead_det_decrypt was incorrectly not labeled STRICT so it crashed on NULL input, I can fix that, or I'm thinking it makes more sense for all functions (in most cases) to throw errors on NULL input instead of returning NULL, which means removing STRICT and doing NULL checking as shown in this branch:

https://github.com/michelp/pgsodium/compare/fix/better-null-checking?expand=1

this is just one example of many that would need to be added. Thoughts?
@ioguix
Copy link
Collaborator

ioguix commented Nov 28, 2022

Hi,

Throwing error from functions seems better:

  • returning NULL without explanation might be tricky for new users to understand
  • it allows a mix of NULL-able args and NOT NULL ones.

@ioguix
Copy link
Collaborator

ioguix commented Nov 28, 2022

I'm not qualified enough with encryption to review your patch, but it seems nonce and additional data are optional with aead_det_*, am I right? See: https://github.com/jedisct1/libsodium-xchacha20-siv#usage

With ietf variant, it seems only additional data can be left NULL, right? https://doc.libsodium.org/secret-key_cryptography/aead/chacha20-poly1305/ietf_chacha20-poly1305_construction#combined-mode

Does it worth sticking with this from the pgsodium point of view?

@michelp
Copy link
Owner Author

michelp commented Nov 28, 2022

Yep, I've pushed some changes to the same that allow for NULL associated with det and ietf and keeping support for NULL nonce with det. It's working well, so I'm going to apply the same pattern across the library.

@michelp michelp linked a pull request Nov 29, 2022 that will close this issue
Fix/better null checking #50
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix/better null checking michelp/pgsodium
2 participants
@ioguix @michelp