fix(arrow-cast): make b64_encode reject invalid UTF-8 from misbehaving Engine impls#10324
Open
bit2swaz wants to merge 2 commits into
Open
fix(arrow-cast): make b64_encode reject invalid UTF-8 from misbehaving Engine impls#10324bit2swaz wants to merge 2 commits into
b64_encode reject invalid UTF-8 from misbehaving Engine impls#10324bit2swaz wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Fixes #10284
Rationale for this change
b64_encodebuilds aGenericStringArrayfrom the output of a caller-suppliedbase64::Enginevianew_uncheckedguarded by a// Safety: Base64 is valid UTF-8comment. That comment only holds for a correct engine.base64::Engineis a safe trait so a caller can implement it in 100% safe Rust to write non-UTF-8 bytes into the buffer. The resultingStringArraythen hands out an invalid&strwhich is UB reached from entirely safe codeA safe fn has to be sound for all safe inputs so this is a soundness hole. It is not a remotely exploitable vuln: the trigger is a caller-supplied
Engine, not untrusted dataWhat changes are included in this PR?
b64_encodenow returnsResult<GenericStringArray<O>, ArrowError>and builds the array through the checkedGenericStringArray::try_newconstructor instead ofunsafe { new_unchecked(...) }. A misbehaving engine now surfaces as anErrrather than UB. This matchesb64_decode, which already returnsResultunsafeblock is gonearrow-jsondoc example)EvilEngine(the issue's repro) that assertsb64_encodereturnsErrb64_decodeis untouched. It returnsGenericBinaryArray, which has no UTF-8 invariant, so this bug does not apply to itVerification
cargo test -p arrow-cast -p arrow-jsonpasses.EvilEnginerepro aborts with UB (char::from_u32_unchecked) against the currentnew_uncheckedcode, and returnsErrwith no UB after this fix.Are there any user-facing changes?
Yes, this is a breaking API change.
b64_encodenow returnsResult, so callers have to handle the error (with?or.unwrap()).There is an alternative: mark
b64_encodeasunsafe fnand document the engine precondition. That has zero runtime cost but pushesunsafeonto every honest caller. I went with theResultroute to keep the safe API safe, but I'm happy to switch to theunsafe fnshape if maintainers prefer it.Credit to the reporter (@Manishearth) for the Miri repro