Skip to content

Fix #72152: base64_decode should reject NUL in strict mode #1917

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

Metabolix
Copy link
Contributor

https://bugs.php.net/bug.php?id=72152

The documentation for base64_decode parameter $strict says, "Returns FALSE if input contains character from outside the base64 alphabet." This should apply to NUL bytes as well.

@nikic
Copy link
Member

nikic commented May 21, 2016

Shouldn't we just drop the \0 check at the top? Even in non-strict mode the NUL byte should simply be ignored.

@Metabolix
Copy link
Contributor Author

That's true, I've updated the PR.

@nikic
Copy link
Member

nikic commented May 21, 2016

With this change ext/standard/tests/url/bug55273.phpt on Travis fails with

006+ bool(false)
006- string(2) "PH"
009+ bool(false)
009- string(1) "P"

This is probably related to the contents in the ch == base64_pad branch ... which look pretty fishy to me. E.g. it increments current in one loop while not also decrementing the length :/

@Metabolix
Copy link
Contributor Author

Whoops. Would you prefer going back to my original fix or revising the whole thing?

@Metabolix
Copy link
Contributor Author

The aforementioned padding branch doesn't even handle the length correctly but instead loops through all whitespace, possibly causing past-the-end access.

I opted for the rewrite, which actually revealed various other problems. I'll start a discussion on the mailing list, let's see how it goes.

@Metabolix
Copy link
Contributor Author

Submitted another approach as PR 1923.

@Metabolix Metabolix closed this May 30, 2016
@Metabolix Metabolix deleted the fix-72152-php56 branch July 22, 2016 17:02
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