Skip to content

Fix memory leak in openssl_sign() when passing invalid algorithm #18185

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 1 commit into from

Conversation

nielsdos
Copy link
Member

Detected using an experimental static analysis I'm developing.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LGTM

@nielsdos nielsdos closed this in 74720a2 Apr 2, 2025
@jrfnl
Copy link
Contributor

jrfnl commented Aug 4, 2025

I have a feeling this commit is causing breakage on PHP 8.5 as the openssl_sign() $algorithm parameter appears to no longer support aliases.

This code:

openssl_sign($signHeader, $signature, $privKey, 'sha256WithRSAEncryption');

Now fails with the following error:

openssl_sign(): Unknown digest algorithm

Found via: https://github.com/PHPMailer/PHPMailer/actions/runs/16728360948/job/47350003979#step:9:33

@nielsdos
Copy link
Member Author

nielsdos commented Aug 4, 2025

@jrfnl I highly doubt it. I can run a bisect in half an hour or so though

@jrfnl
Copy link
Contributor

jrfnl commented Aug 4, 2025

@nielsdos That would be appreciated!

I agree that based on the code in this PR it seems unlikely, but it was the only commit I could find for PHP 8.5 which related to the openssl_sign() method and the last passing build in PHPMailer is from a couple of days before this was merged and the first failing one after this was merged, so the timing seems right, which is why I left the comment.

@jrfnl
Copy link
Contributor

jrfnl commented Aug 4, 2025

@nielsdos Looking more closely - you're right, it can't be this commit - the (failing) CI builds from a PR were confusing the issue.

Last passing is actually on May 31st, first failing on June 18th. Still mystifying though why the alias no longer works. I couldn't find anything in UPGRADING or NEWS related to this.

Shall I open a bug report instead ?

@nielsdos
Copy link
Member Author

nielsdos commented Aug 4, 2025

@jrfnl It can also be an environment change, e.g. an update to the OpenSSL library itself. You may open a bug report.

@nielsdos
Copy link
Member Author

nielsdos commented Aug 4, 2025

Nope, works in 8.4 breaks in 8.5, I'll bisect...

@jrfnl
Copy link
Contributor

jrfnl commented Aug 4, 2025

@nielsdos I've gone through all commits in PHP master between May 31st and June 18th and my current suspicion is that this is the culprit: #18516

I previously already verified via var_export(openssl_get_md_methods(true)); and the alias still exists and the output is not really different between PHP 8.4 and 8.5.

@nielsdos
Copy link
Member Author

nielsdos commented Aug 4, 2025

@jrfnl You're correct, bisect finished and points to 2f5ef4d

@jrfnl
Copy link
Contributor

jrfnl commented Aug 4, 2025

@nielsdos Thank you for doing that and confirming! In the mean time, I've set up a phpt test for alias support in openssl_sign() - I'm waiting to verify the test works as intended and if so, I'll include it in the bug report.

@jrfnl
Copy link
Contributor

jrfnl commented Aug 4, 2025

@nielsdos I've opened the ticket now: #19369

@ratatatKE
Copy link

Great job @nielsdos for catching this memory leak. Are you planning to opensource the experimental static analysis tool that you used to catch this one?

@nielsdos
Copy link
Member Author

nielsdos commented Aug 4, 2025

Great job @nielsdos for catching this memory leak. Are you planning to opensource the experimental static analysis tool that you used to catch this one?

That's the plan eventually, the paper describing the analysis tool is currently under review

@ratatatKE
Copy link

Great job @nielsdos for catching this memory leak. Are you planning to opensource the experimental static analysis tool that you used to catch this one?

That's the plan eventually, the paper describing the analysis tool is currently under review

Okay, where can I get my hands/eyes on the pre-release? Is it something online I can find?

@nielsdos
Copy link
Member Author

nielsdos commented Aug 5, 2025

Okay, where can I get my hands/eyes on the pre-release? Is it something online I can find?

It's not public yet, it will only become public once the journal publishes the article and source code (after it got reviewer approval).

@ratatatKE
Copy link

Okay @nielsdos, thanks for the update. Looking forward.

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.

5 participants