client: fix nil-part panic in MultipartDeserialize on malformed body#70
Closed
SAY-5 wants to merge 2 commits into
Closed
client: fix nil-part panic in MultipartDeserialize on malformed body#70SAY-5 wants to merge 2 commits into
SAY-5 wants to merge 2 commits into
Conversation
MultipartDeserialize() handled r.NextPart() by only checking for io.EOF
and otherwise using the returned *Part unconditionally. When the caller
declares Content-Type: multipart/related but the body is not valid MIME
multipart data (missing/malformed boundary, truncated body, plain text,
etc.), NextPart() returns (nil, <non-EOF error>). The next line
dereferenced nil via part.Header.Get("Content-Type") and panicked the
goroutine handling the request.
The request path starts unauthenticated (POST /nsmf-pdusession/v1/sm-contexts
on SMF and similar MultipartRelatedBinding paths on every NF that uses
this library), so any attacker reaching the SBI port can deterministically
crash the handling goroutine.
Return the NextPart() error to the caller instead of reading nil.Header.
Also fix the part.Read() error check that was inverted (compared err to
nil when it should return on err != nil and tolerate io.EOF).
Refs free5gc/free5gc issue 1026.
Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
Contributor
Author
|
Done, trimmed verbose comments and moved context to the PR description. Thanks @roundspring2003! |
Contributor
|
@SAY-5 plz help fix the conflict |
Contributor
Author
|
Thanks for the ping. Checking against current main: the nil-part guard and the MultipartDeserialize error wrapping from this PR are already present verbatim in client.go, so the change has effectively landed through another path. Rebasing here would only produce empty commits. This PR can be closed as superseded unless you want it kept for tracking. |
SAY-5
force-pushed
the
fix/multipart-nil-panic-free5gc-1026
branch
from
1b4bc13 to
11361a5
Compare
Contributor
Author
|
Superseded by 615098a (MultipartDeserialize: guard against nil part on malformed body) on main, which already adds the same nil-part / non-EOF-error guard in MultipartDeserialize. Closing as the fix is upstream. |
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
2 participants
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.
Refs free5gc/free5gc#1026.
Problem
MultipartDeserializeinclient.goonly checkedr.NextPart()forio.EOF. When the declaredContent-Type: multipart/relatedbody is not valid MIME (missing/malformed boundary, truncated body, plain text, empty)NextPart()returns(nil, err), and the subsequentpart.Header.Get("Content-Type")dereferences nil and panics the handler goroutine.Because the shared openapi client lives behind every NF's
MultipartRelatedBindingpath, an unauthenticated attacker reaching the SBI port (e.g. SMF'sPOST /nsmf-pdusession/v1/sm-contexts) can deterministically crash the handler with a single malformed POST.Fix
NextPart()instead of dereferencingnil.part.Readerror check (wasif err == nil { return err }) so a legitimate short read doesn't silently return a nil error and so unexpected read errors bubble up.Test
go build ./...andgo test ./...clean. The package has no existing tests forMultipartDeserialize; happy to add a regression test here if maintainers prefer.