Skip to content

ext/sodium: sodium_crypto_aead_(aes256gcm/aefis128l)_decrypt adjustments #17588

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

Closed
wants to merge 3 commits into from

Conversation

devnexen
Copy link
Member

a size_t could not be greater than SIZE_MAX here.

a size_t could not be greater than SIZE_MAX here.
@nielsdos
Copy link
Member

@devnexen Please see Slack.

@devnexen devnexen marked this pull request as draft January 26, 2025 15:33
to potentially protect against buffer overflow.
@devnexen devnexen marked this pull request as ready for review January 26, 2025 15:49
@nielsdos
Copy link
Member

This looks good already, but it seems we have a whole bunch of checks involving SIZE_MAX and string lengths. It's highly likely all of them should be changed to ZSTR_MAX_LEN (but check this).

@cmb69
Copy link
Member

cmb69 commented Jan 26, 2025

I think we should coordinate with upstream.

@nielsdos
Copy link
Member

If that's the case, then upstream is already behind with what is in php...
Since ext-sodium is bundled I would consider this one the upstream imo.

@nielsdos
Copy link
Member

There's still a few of the form SIZE_MAX - msg_len or subkey_len > SIZE_MAX

@cmb69
Copy link
Member

cmb69 commented Jan 26, 2025

If that's the case, then upstream is already behind with what is in php...

I think both implementations have diverged over time. E.g. search for zend_string_checked_alloc. Anyhow, I don't particularly mind.

@devnexen devnexen closed this in d6c6675 Jan 26, 2025
@devnexen devnexen deleted the libsodium_adjust branch February 16, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants