Security & concurrency hardening for the Wi-Fi transfer feature#1
Merged
Merged
Conversation
The PIN is the only authentication for the upload endpoint, but the /transfer/info handler returned it in plaintext to any unauthenticated caller on the LAN, letting anyone read it and then POST to /transfer/upload. The sender never consumed this field anyway. Remove the token from the info response; the PIN is now only shown on the receiver's screen and must be entered manually on the sender. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The server buffered the entire HTTP request in a std::string driven by an attacker-controlled Content-Length, and parseMultipart then copied the whole body and the file part again (~3x the file size resident at once). On the 3DS's small heap this could OOM/crash on large saves or via a crafted Content-Length. - Cap the buffered request at MAX_REQUEST_SIZE (32 MiB) and reply 413 when exceeded, also guarding against a client that never terminates headers. - parseMultipart now returns the file part as an [offset,length) range into the original buffer instead of copying it, so the payload exists only once in memory and is written straight to disk. Also drops a stray trailing-CRLF trim that could truncate binary payloads ending in 0x0D0A by two bytes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The server runs on a single thread and the accepted socket is blocking, so a client that connects and then stalls would block the receiver forever: stopReceiver() only unregisters handlers and never closes the socket, and no further connections could be accepted. Set SO_RCVTIMEO (15s) on each client socket so a stalled recv unblocks, the request is abandoned, and the server returns to accepting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
g_transferMode (a std::string) and the g_transferBytes{Done,Total}
counters are written by the HTTP server thread while receiving and read
by the UI thread when drawing progress. Concurrent access to the
std::string is undefined behavior (and can crash), and 64-bit reads tear
on the 3DS's 32-bit core.
Route all access through mutex-protected helpers (transferSetMode/
transferGetMode/transferSetProgress/transferAddDone/transferGetProgress)
and protect the receiver notice/completed-name strings with their own
mutex behind setReceiverNotice/setReceiverCompletedName and locked
getters. UI readers now take a consistent snapshot under the lock.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Transfer start/stop now registers and unregisters HTTP handlers at runtime (main thread) while the network thread concurrently looks them up, racing on the std::map. Guard all access with a mutex; the request dispatcher copies the handler out under the lock and releases it before invoking the handler so a long upload doesn't block (un)registration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The top-screen transfer loader drew the mode label, percentage and byte counts as one centered line, so longer labels like "Preparing backup package" ran off-screen and didn't match the new modal loader. Split it into two centered lines (label/percent, then size) like the bottom modal. Also format the transferred/total size in MB across the top loader, the bottom progress modal, and the receiver overlay instead of raw bytes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
These commits address review findings on BernardoGiordano#528. Merging this into your branch will fold the fixes into that PR.
Each fix is a self-contained commit:
/transfer/info— the endpoint returned the token to any unauthenticated caller on the LAN, defeating the only auth on the upload endpoint. The PIN is now only shown on the receiver's screen.parseMultipartreturn an offset/length into the original buffer instead of copying the payload twice (~3×→1× RAM). Also drops a stray CRLF trim that could truncate binary payloads ending in0D 0A.poll()because the 3DS SOC layer doesn't supportSO_RCVTIMEO.g_transferMode, the byte counters, and the receiver notice/name strings were read by the UI thread while the server thread wrote them (UB on thestd::strings, torn 64-bit reads). Now behind mutex-protected accessors.handlersmap — it's now mutated at runtime (transfer start/stop) while the network thread reads it; added a mutex.Builds cleanly with
make 3ds.Note: the 4-digit PIN is intentionally left as-is (the sender obtains it out-of-band from the receiver's screen). Remaining low-severity nits were posted as inline comments on BernardoGiordano#528.