Harden Seos Compatible - #20
Merged
Merged
Conversation
Compiles the app's own sources against furi and bit_buffer shims so the protocol and crypto code can be tested on a workstation. Starts with CMAC against known answers. The bit buffer shim bounds-checks and aborts on overflow, matching furi_check on device, so overruns show up as test failures.
SEOS_APP_EXTENSION builds and matches paths, so widening it to ".seos|.bin" made saving write "<name>.seos|.bin", left Delete unable to match any file, and broke duplicate-name detection. The browser filter now has its own macro.
Every length in a secure message comes off the wire, and none of them were checked. A short frame claiming a long cryptogram read past the buffer, an empty one underflowed the padding loop, and a length that was not a whole number of blocks left the plaintext buffer uninitialised. One helper now parses both directions: it checks the tag, the length form, block alignment and the buffer bound, and requires the mandatory pad byte, so a wrong key shows up as a refusal instead of garbage plaintext. Unwrap returns whether it succeeded, and callers check before parsing what they hope is plaintext. Also fixes the response wrap dropping the pad byte when the message was an exact block multiple, a one-byte write past the plaintext buffer at maximum length, and cryptogram lengths of 128 and above being written as a bare byte. The cipher wrappers now report failure instead of silently leaving the output zeroed.
Both unwrap paths carried a note that the checksum was never checked, and neither ever did. Any message could be altered in flight, and a session with the wrong keys handed back whatever the ciphertext decrypted to. Received messages are now checksummed before they are decrypted, over the same scope the wrap side uses -- one function builds it for both, so the two cannot drift apart. A message whose checksum is absent records 6987 and one that fails to match records 6988, for the caller to answer with. Tampering with the cryptogram, the command header or the checksum itself is rejected, and so is a replayed message, since the sequence counter has moved on by then.
Every role filled its session nonces with a constant, so every session derived the same keys and started the sequence counter at the same value. Any recorded exchange replayed. They now come from the hardware random source. The derivation also stepped by zero for a hash it did not recognise, which spun the loop forever. It refuses the parameters instead, and callers that used the session straight away check for that. Adds vectors for both derivations, taken from a model of the construction written separately.
Selecting an application, agreeing a session key and the cryptograms on either side of it sat in the same files as the NFC poller and listener callbacks, so nothing but NFC could reach them without dragging the radio in. Both BLE stacks already called across that line. They move to seos_protocol.c unchanged, which also makes them testable: the reader and card halves now run against each other and are shown to reach the same session keys, for both ciphers. The second authenticate step ignored the frame length and read a fixed cryptogram out of it, so a short frame read past the end. It checks first.
The four transports each carried the same thirty lines, so every fix had to be made four times, and they had already drifted: only one logged an unmatched request, only one answered a status word after it, and the native peripheral leaked its working buffer on the way out. There is one copy now. It reports what happened through a callback instead of reaching into the view dispatcher, which also gets the UI out of the protocol path. The command header the four had each declared separately is one constant. Behaviour follows what the emulator did. The transports that answered a status word after an unmatched request no longer do; the next change gives every command a proper answer.
Anything other than the one literal tag list for the SIO file got no answer at all -- not a status word, not an empty frame, nothing. A reader asking for any other object waited for a reply that never came. The data field is now parsed as a tag list. An object the card does not hold is answered with success and no data, a data field that is not a well formed tag list with 6a80, and an extended header list likewise, since we do not serve those. A secure messaging error is answered in the clear and ends the session, which the transports now act on by dropping it. Responses carry the status word they are answering with rather than a hardcoded success, and a response with no data omits the cryptogram instead of encrypting nothing.
A SIO much over a hundred bytes could not be carried. The card either tripped a bounds check building the frame or silently sent nothing, and the reader took whatever arrived as the whole answer. The card now protects the whole response as one message and hands it out a frame at a time, ending each piece with 61xx while more is left. The reader collects the pieces before unwrapping, since a piece of a protected message means nothing on its own. A new command drops a response nobody collected, and the reader's loop is bounded so a card that never finishes cannot hold it. The cryptogram limit goes up to fit a full SIO plus the file id and length that precede it.
Only the read instruction was matched, so a write got the same silence any other unrecognised command did. Both instructions on the current file now reach the handler, which dispatches on the instruction byte. A write stores what it carries, bounded by the room there is rather than the length the command claims, and reports what happened so the scene can offer to save it.
The native peripheral's worker used the stop flag the external dongle path's UART worker declares, and reached it through a chain of includes that put the whole external stack in front of every file in the app. It has its own flag now, and the app header no longer pulls that stack in. The app struct was allocated without being zeroed, so the three BLE pointers started as whatever was on the heap, and the peripheral scene reads two of them on the way out. Two more members nothing has ever read are gone; the real ones live in the peripheral. Structural only. The suite still passes, and there is no new behaviour here to test.
A FAP is resident in memory in its entirety while it runs, and roughly a third of this one was BLE that most sessions never touch. Worse, the larger half of it -- the dongle stack -- cannot run at all in a shipped build, since the flag that selects it is never set. Both stacks are now built as plugins and shipped inside the .fap, loaded when a scene needs one and unloaded when it leaves. The sources move across unchanged: everything they reach back for is resolved through the app's own symbol table rather than being passed in, so there were no call sites to rewrite. Crypto stays on the app side, because a plugin that called mbedTLS would have to carry its own copy. Teardown order is load-bearing and now explicit: stop the plugin, free its context, then unload it. A callback re-entering unmapped code is a hard fault. The app works with the plugins absent -- the scene says so and nothing asserts. Also moves the chunk framing the two stacks share out of one of them, and the dongle's power rail into the plugin that uses it rather than the whole app lifetime. Shared headers no longer pull in the generated icon header, which the plugins have no use for. The resident image drops from 71,628 to 49,592 bytes.
The flag selecting the dongle stack was hardcoded false and never written, so in any shipped build the scanners menu was hidden and that whole stack was dead code. The peripheral scene now tries the dongle first and falls back to the Flipper's own radio, recording which it got. The scanners menu is offered whether or not a dongle is attached, since whether one answers is not known until the stack is loaded, and the scene says so when it does not. The dongle plugin refuses to start if the serial port is held rather than asserting inside its worker thread, and gives the power rail back when it does. This is a check that the port is free, not a handshake with the dongle; whether one is really there still shows up through the link events the scene already displays. The same gate is still on "BLE Emulate Central" in the saved menu, which has uncommitted work in it.
The checksum input was assembled in a heap buffer allocated and freed on every message, which bought nothing over a stack array. It is now bounded explicitly rather than relying on a capacity check that would have aborted. The two log helpers each built a two-hundred-and-fifty-byte string a byte at a time before the log call threw it away at the default level. They return early instead. They run on every message in both directions. Key derivation no longer logs the derived key. It is key material, and the log is not the place for it. The keys menu held a quarter-kilobyte filename buffer on the stack for names bounded at thirty-two characters.
demo.mkv is nine megabytes of the same recording demo.mp4 holds, and nothing refers to it. The mp4 stays: the README links it. dist/ turned out never to have been tracked, so there is nothing to remove there.
Offering the dongle scanners to everyone was wrong: most people have no dongle, and the menu should not pretend otherwise. Whether one is attached is something the user knows and the app cannot cheaply find out, so it is a setting. "External BLE" on the main menu turns it on, and is saved. With it off the scanners are hidden, the peripheral role uses the Flipper's own radio, and nothing ever asks for the dongle plugin, so its stack costs nothing. With it on the dongle is preferred for the peripheral role and is the only thing that can serve a central one. The rule deciding which stack serves which role is its own small unit so it can be read and tested apart from the loading, which is where this went wrong before: the dongle stack could never be chosen at all. has_external_ble now means the setting, which is also what the gate on "BLE Emulate Central" in the saved menu has always assumed it meant. The settings file itself is not covered by the suite; it needs storage, which the host harness does not stand in for.
The dongle is off until it is turned on, which nothing said. Also records this round of changes in the changelog.
A credential read over the external BLE reader saved with no keys and no ADF OID, so it could never be emulated afterwards. The NFC reader and the native BLE reader both keep them; this path is a copy of those that never picked the lines up. This is the drift the shared command handler was meant to stop, still living in the reader flows, which are not shared yet.
aes_cmac.c and des_cmac.c were the same algorithm twice, differing only in block size and in the constant the subkeys are derived with. A fix to one would not have reached the other. mbedTLS has a CMAC of its own, but not in the Flipper's build: cmac.c is not compiled and its header is not shipped. furi_hal_crypto has no MAC over a caller-supplied key and no 3DES at all. So the algorithm stays here, once. Cipher failures now propagate instead of being discarded during subkey generation and the block loop, and the block count is no longer a byte, which silently wrapped past four kilobytes of input. The known answers covering both ciphers pass unchanged.
Four copies, character for character identical apart from the function name, turning the same two events into the same two view dispatcher events. All four were already handed the Seos* they needed. Declared apart from seos_i.h so the plugins can see it without the GUI headers coming with it.
The same three-armed ladder -- AES, then 3DES, then a warning -- appeared about a dozen times across the protocol and the secure channel, and the four mbedTLS context dances differed only in which cipher and which direction. They are four helpers now. A cipher we do not know returns false rather than falling through the ladder and leaving the output buffer as it was, which is what several of the copies did: the caller could not tell nothing had happened.
Four copies of the reassembly prologue and three of the chunk loop, all of them the same. Two of the four allocated their working buffer before the guard and returned through it without freeing, so every incomplete frame and every error frame leaked one. Moving the guard into a helper makes that shape impossible: the caller either has a whole message or has returned. Reassembly also refuses a message longer than the buffer collecting it, which none of the copies checked -- they appended and let the bounds check abort. Both plugins compile the shared source rather than resolving it through the app, so it stays out of the resident image.
Five copies of the same loop calling snprintf once per byte, three of them outside any log-level guard so they paid for it whether or not anything was listening. The toolbox has uint8_to_hex_chars, which is a table lookup and is exported to apps. The bit buffer helper is now a wrapper over the plain one rather than a second copy, which also settles a disagreement between them: one reported the full length beside a truncated string, the other the truncated length. Output is uppercase now, the toolbox helper's convention. Debug logs only.
Three transports carried their own success and file-not-found bytes, which shadowed the ones the protocol already exports, so a change to a status word would have had to be made in four places. The secure messaging error was written out as two literal bytes in eight more. They all use the exported constants now, appended through one helper rather than a pair of shifts at each site.
The dongle transport ran a second thread whose whole job was to wait for a flag and then call furi_hal_serial_tx, which waits on the transfer itself. That cost a kilobyte and a half of stack and a 256 byte staging buffer to do nothing the caller could not. It also raced: the sender refilled the staging buffer with no lock while the thread could be reading it. There was a semaphore for exactly this, allocated and freed and never once taken. Sending straight from the caller's buffer removes the copy and the race with it.
Four scenes that are nothing but a popup and a timeout carried the same forty lines each, differing in an icon, a caption and where they went afterwards. They are a config struct and three calls now. Zero keys keeps its own event handler: it goes on to the main menu rather than back to it, which is a different thing and worth not hiding.
Seven scenes carried the same eight lines turning a button press into a custom event. Two of the seven had dropped the assertion on the context the other five kept.
The previous commit swept in an unrelated working-tree change to this file that was not mine to commit. This puts the file back as it was; the change returns to the working tree, uncommitted.
The menu is rebuilt on every entry, so it needs resetting first or the items accumulate. The remembered selection could also name an item that is not in the list, since "BLE Emulate Central" is only added when the dongle is turned on; a selection outside the list falls back to the first item.
Four defects that would each break a real exchange: The authenticate command was matched with the keyset number baked in, so a reader using any keyset but one was ignored entirely and got a bare success with no data. The keyset is a parameter of the command, not part of its identity; it is read rather than matched, and the reader builds the command for whichever keyset it wants instead of patching a shared global. The emulator appended a success word to any response whose last two bytes were not already one it recognised. That turned a chaining status into a chaining status followed by success, which ends the exchange early, and turned every error into an error followed by success. Handlers that produce a complete answer now say so, and the fallback is only for the ones that do not. The continuation command was sent without its expected-length byte, which a strict card refuses outright. Object lengths were written and read as a single byte. The stored credential can be a hundred and twenty-eight bytes, at which point a bare byte is read back as a length header rather than a length. Both directions now use the long form above a hundred and twenty-seven. Also replaces the message authentication known answers with ones of our own, computed by two implementations written separately from this one.
Three ways an emulated card told a prober more than a real one would. The initialisation vector in the select answer was built from a fixed all-zero value, so every answer for a given credential was byte identical. It is a fresh random half-block now, and the payload is padded with random rather than zeroes so its true length does not show through. An unmatched application identifier returned file-not-found, which tells anyone who asks exactly which credentials the card does not carry. A failed authentication returned nothing at all, which is a direct answer to "was my key right". Both now answer with well formed nonsense and success, which without the key is indistinguishable from the real thing. The host random source used to hand back the same bytes on every call, which would have let all this pass. It advances now, and still starts from the same place each run.
A card that answers 6Cxx is saying the length we asked for was wrong and naming the right one. We treated that as a refusal and gave up. It is worth one more try. The retry sends exactly what went out with only that one byte changed. Building the command again would step the sequence counter, and the card would then refuse a message it had no reason to refuse. The decision about what each status word means now sits where it can be tested rather than inside the exchange loop, which needs a radio to run. Nothing this reader sends should provoke 6Cxx -- it asks either for as much as the card has, or for exactly what the card just offered -- but a card is free to send it anyway.
A file states how long its diversifier, credential and application identifier are, and each of those numbers was handed straight to the read as a count into a field of fixed size. A file claiming more than fits overran the field, which for the two larger ones means writing past the end of the credential. The lengths are checked before they are used now, and a file claiming more is refused. The saved select answer had the same shape of fault at the other end: its length came out of the saved bytes, was not checked against the field holding them, and the result went on the air. A saved answer claiming more than fits is now ignored and one is built instead. Left alone, a damaged credential would have transmitted a hundred and ninety-one bytes of whatever followed it in memory. The exchange this all came from is now a test: the reader's half of an exchange, replayed, has to produce the card's half byte for byte -- the same select answer, challenge, cryptogram and protected response. It does. Adds the tool that decoded that capture, and stops the source glob picking up the bytecode cache it leaves behind.
Grouping the length checks moved one read above another, and a key is looked for from the current position onward with no rewind. Asking for the credential length first stepped the cursor past the diversifier, which could then never be found, so every file failed to load. Each length is read, checked and used in the order the file lists it. The checks themselves were right; only where they sat was wrong. Drops the helper that grouped them, which no longer matches how the loader works. Coverage for this comes next, from tests that run the parser itself.
Region coverage was 76% with three protocol functions never run at all, the reader's side of selecting an application among them at sixty uncovered regions. The card builds a select answer and the reader takes it apart again, for both ciphers, so the two are checked against each other rather than against a recorded blob. That reaches the payload builders, the reader's parser and the refusals: an answer too short, one with the wrong header, one whose contents do not describe an application, one longer than the field that holds it. Also covers the authenticate matchers, which decide whether a command is answered at all, and the cipher wrappers including the lengths and the ciphers they refuse. Adds `make coverage`, measured over the app's own sources only, since counting the shims and the tests would flatter it. Regions 76% to 86%, lines 81% to 91%, and every function is now reached by something.
The replay and the session cases carried a working credential: its two keys, its stored payload and its saved select answer. None of that belongs here. They use credentials invented for the purpose now. What made those tests worth having is kept: the card's answers are still worked out by a model written separately from this implementation, so they check it against something other than itself rather than recording whatever our own code produced. The larger of the two session cases now carries enough to need a long-form length, so both forms are covered, and the assertion is about the form rather than one particular length.
Two problems only appeared under gcc on Linux. mbedTLS renamed the hash calls between releases: the plain names are deprecated in 2.x and correct again in 3.x. Only 3.x carries a version macro into the hash headers, so its absence selects the older spelling. memmem is hidden behind _GNU_SOURCE on glibc, so the call compiled with no declaration in scope. The flag lets the C library declare it.
Four copies of the same encoding had grown up: two in secure messaging, two in the command handler, each handling the short form and one long-form octet and nothing else. One module now does it, working from a buffer and a length so a caller can walk a message header by header and so the parsing can be tested away from any transport. It reads the forms X.690 defines that this protocol uses -- tags of one or two octets, lengths of up to two octets -- and refuses the rest rather than guessing. Writing always picks the shortest legal form. A length written longer than it needs to be is still read. Not every encoder on the other end is minimal, and refusing them would drop messages that are otherwise well formed.
The reader parsed responses inline, between poller calls, so none of it could be reached by a test. Four parsers now take a buffer and a length and live with the rest of the exchange, and the reader calls them. Three things were wrong and are fixed by the move: A credential of 128 bytes or more was copied from the wrong offset. The long form puts the value one byte further along, and the length was read there but the copy still started where the short form would have put it, so every byte came back shifted. Four status word checks stepped two bytes back from the end of a response without first checking there were two bytes to step back over. So did the comparison against the select answer. The second authenticate answer was matched a byte at a time, which accepted a wrapper whose length disagreed with what followed it.
A frame shorter than the header its own PCB describes made the remaining length count backwards past zero, and the enormous result was then used as a length. The emulator did this in both flow modes, and neither checked there was a command there at all before comparing fifteen bytes of one. One function now works out where a command starts and how much of it there is, and refuses a frame that carries only a header. The three places that had each done their own arithmetic call it. The answer to a select was worse. It read a length from a fixed offset and subtracted the vector size from it, so a short length wrapped and the cipher was handed a size far past both its input and the 64 byte buffer it decrypts into. The offsets it then read the identifier and diversifier from were unchecked as well. It now reads the objects as objects, and every length is bounded by what was actually decrypted. One caller passes an offset of 1, so this was reachable over BLE. The matcher for the first authenticate command was one byte short in its own guard: the key number sits between the header and the body, and it counted only the two. Added a sanitised run, `make test-asan`. The truncation sweeps only proved a parser returned false; under the sanitiser they also prove it did not read past the buffer to decide that. It found the matcher bug on the first run.
The same arithmetic as the NFC side, in four more places. Each read a fixed distance into a message before checking there was a message that long, and each counted a remaining length by subtracting a header from a size that might be smaller than it. The attribute handler read its opcode from an empty payload and took a write request header off three messages without checking one was there. The channel layer read a length and a channel out of a header it had not confirmed it had, then copied a payload longer than what arrived. Both Seos flows skipped a byte of framing and compared up to fifteen bytes of what followed. The peripheral had its own copies of the two authenticate answer parsers, both matching a byte at a time. They are gone: it calls the ones the reader uses, which check lengths and were already covered. None of this is in the host build, so none of it is under test. The guards are the same shape as the ones that are.
The write path checked its answer for a success word in the clear and believed it. Nothing about that is protected: any card, or anything else in the field, can send two bytes. The checksum went unchecked and the protected status was never read. It also left the session counter where it was. The card steps its counter for the response whether the reader looks at it or not, so the two were out of step from that point on and every later command would have been refused. The answer is now unwrapped, which verifies the checksum, reads the protected status and steps the counter. A tampering sweep covers the protected part of the answer a byte at a time. The status word in the clear is deliberately outside it: the tests pin that changing it does not alter the verdict in either direction, because the protected one is what is believed. The command that write sends builds its object with the shared encoder instead of its own copy.
Parsing lived inside the loaders, between opening a file and showing a dialog, so nothing could reach it from a test. It now takes an open file and the struct, and the loaders call it. Fields may appear in any order. The reader underneath searches forward from the cursor and never rewinds, and a miss leaves it at end of file, so every read now rewinds first. Before this, reading two fields in an order the file did not happen to use lost the second, and one absent optional field consumed every field listed after it. The mock reproduces the forward-only search and the cursor left at end of file. A mock that found keys in any order would accept code the device rejects, so the tests cover both the parser and that behaviour. Comments in the modules added this session reworded for plainer style.
A reader storing a credential on us raised the same event a successful read of a card raises. The emulation scenes answer that event by offering to save, prefilled with the path the credential was loaded from, so being written to was one keypress from overwriting the file. The two are opposite directions and should not share a meaning. A write now raises its own event. The three scenes that emulate a credential write it back to the file it came from and say so. A credential that was never loaded from a file has no path of its own and is left alone. The custom event enum moved to its own header so the mapping from secure messaging events can be tested without the GUI headers. A test pins that a write does not raise the read event.
Wrapping and unwrapping used four fixed arrays on the stack: two of 192, one of 208 and one of 256, about 850 bytes charged to every call whatever the message, and a hard ceiling at 192. They are now sized from the message. The ceiling is gone for responses, which carry no length of their own. A command still states its own length in a single byte, so its objects and checksum have to fit in 255. That works out to 223 bytes of message for either cipher. Past that, wrapping refuses rather than sending a length that wrapped around, which the fixed buffers had made unreachable and heap sizing did not. The emulator asked for 128 byte frames regardless of what it had advertised. The card data says 256, so it was chaining twice as often as it needed to. It now takes the frame size from what was advertised and subtracts the block header and checksum. Not yet confirmed against a reader. Two of my own tests were wrong and are fixed: both stepped the sequence counter by hand before wrapping a response, which wrapping does itself, so they were passing or failing on drifted counters rather than on what they claimed to test.
The listener answered by copying the received header back. That is right by accident for a plain I-block, because the block number sits in a bit that survives being copied, and wrong for everything else. A supervisory block asking for the last answer again, or asking to end the exchange, was passed on as though it were a command. A frame is now classified first: an S-block is answered with the same S-block, an R-block with an I-block, and only an I-block goes on to be read as a command. A chained command is refused rather than half handled, since nothing here sends one. The reply PCB is built from the received one, carrying back the block number and the card identifier and nothing else. The node address bit was wrong, and I introduced it earlier in this series by taking the name from the constant already here. It is bit 2, not bit 3, which is the card identifier. A frame carrying a node address was read one byte out of place, and one carrying both was read two out. Header length now counts both, and only on an I-block, where they mean anything.
The five items are done. Neither the larger frame size nor the block replies have been confirmed against a real reader yet, so that is what is left.
bettse
reviewed
Aug 29, 2026
| seos_reader_generate_cryptogram, | ||
| void, | ||
| (SeosCredential*, AuthParameters*, uint8_t*)), | ||
| API_METHOD(seos_reader_verify_cryptogram, bool, (AuthParameters*, const uint8_t*)), |
Owner
There was a problem hiding this comment.
testing of "Start BLE Reader" and "... -> BLE Emulate Peripheral" indicate these is missing
+ API_METHOD(seos_is_general_authenticate_1, bool, (const uint8_t*, size_t)),
+ API_METHOD(seos_is_general_authenticate_2, bool, (const uint8_t*, size_t)),
+ API_METHOD(seos_build_general_authenticate_1, void, (uint8_t, uint8_t*)),
+ API_METHOD(seos_parse_ga1_response, bool, (const uint8_t*, size_t, uint8_t*, size_t)),
+ API_METHOD(
+ seos_parse_ga2_response,
+ bool,
+ (const uint8_t*, size_t, const uint8_t**, size_t*)),
Collaborator
Author
There was a problem hiding this comment.
Good catch, and thanks for testing it on hardware — that was the only place it could show up.
All five were mine. seos_is_general_authenticate_1/_2 and seos_build_general_authenticate_1 became plugin dependencies when I split the protocol code out of the transports, and seos_parse_ga1_response/seos_parse_ga2_response when I changed the native peripheral to call the reader's parsers instead of keeping its own byte-at-a-time copies. I added the calls and never added the exports.
I'd also seen the .fal "Symbols not resolved" warning during builds and talked myself out of it, on the grounds that fbt can't see the app's own table so the warning lists everything. That's true, but it meant I never checked the one thing it was actually right about.
Added the five in 2553b82, plus make check-plugin-api, which compares what the plugin sources call against what the table exports. CI runs it now. Removing an entry makes it fail, so it's checking something rather than just passing.
One thing I have not verified: I can't reproduce your on-device test here, so this is confirmed statically only. If you get a chance to re-run "Start BLE Reader" and "BLE Emulate Peripheral" on the new head I'd appreciate it.
Owner
|
tested reading and some native ble, didn't test nfc emulation or external ble since that requires some setup. One comment, but generally looks good. I gave you write to the repo, since you've contributed a bunch to seader, s feel free to merge this when you're ready |
The plugins resolve the app's functions through the table in seos_app_api_table_i.h. Five they call were never added to it, so both stacks failed as soon as they were used: the three authenticate matchers when the protocol code was split out, and the two response parsers when the peripheral was changed to share the reader's. Nothing catches this at build time. A missing symbol compiles and links clean on both sides and fails only on the device, which is where it was found. `make check-plugin-api` compares what the plugin sources call against what the table exports, and CI runs it. Removing an entry makes it fail, so it is checking something. Also corrects the declared extent of SEOS_GET_RESPONSE, which said four bytes where the array is five. The symbol hash is over the name alone, so this was misleading rather than broken.
load_path records the file a credential was loaded from. Save writes beside it, delete removes it, and a write from a reader persists to it. The file browser was handed that same string as its starting position. It is written on cancel as well as on a choice, so backing out of the browser left a directory there, and the three callers above would then have acted on a folder. The browser gets its own string now, seeded from load_path when there is one and from the app folder otherwise. load_path is updated only when a file is actually chosen. Passing one string both ways is also what the firmware's own callers do, and it makes the browser's current folder agree with its base path, which is the condition under which back closes it rather than trying to move up a level.
Every length below arrives from the other end and was used without being checked against what actually arrived. SELECT ADF took its OID list length from the command and handed it to memmem with no check that the command carried that many bytes, or that it was long enough to hold the length byte at all. Four copies of that, one per transport. One bounded parser replaces them, and the four local copies of the command header go with it. GENERAL AUTHENTICATE (2) was passed the whole frame size against a pointer that starts past the block header, so the length overstated the APDU by the header and the cryptogram could be read off the end of it. I fixed the other call sites for this and missed these two. The secure messaging matcher was called with a constant four in all three BLE transports, so its own length guard never fired. A GATT write copied a peer-stated length into a 243 byte buffer with no clamp. L2CAP appended a continuation into a 256 byte accumulator and only then reported it was too much, which asserts and takes the app down. It is checked before the append now, and a start frame claiming more than can ever be assembled is refused rather than begun. The plugin API check caught the new symbol missing from the table, which is what it is for.
secure_messaging_alloc wrote to whatever malloc returned. On a device with a small heap that is a null dereference at the point a session starts. Freeing a session released the struct holding both session keys and the two cipher contexts without clearing them, so the key material stayed in the heap for whatever allocated next.
Both BLE credential paths answered a GENERAL AUTHENTICATE (2) that did not verify by reporting Authenticated and deriving session keys from nonces the card never proved it held. Both BLE reader paths did the same when the card's cryptogram came back the wrong length -- including the length check I added, which logged and then allocated anyway. Only the NFC path and the central already refused. They now answer as NFC does, with well formed nonsense that gives away no more than a refusal, and start nothing. A second authentication also overwrote the session pointer without freeing the first, and none of the three BLE contexts freed it on teardown. The external reader parsed a card's challenge with a fixed memcpy after a one byte minimum. It uses the same bounded parser the native one does. Selecting an application read ten bytes of identifier at a fixed offset with no check the command was that long, in four places. One bounded parser replaces them, and the local copies of the command header go with it.
A read answer longer than one frame arrives in pieces, each ending in 61xx to say more is coming. Only the NFC reader knew that. Both BLE readers unwrapped the first piece as though it were the whole message and read the credential as a two byte tag, a one byte length and a value, so a credential needing more than one frame was truncated, and one of 128 bytes or more was misread even when it fitted. The host tests already have a 120 byte credential that does not fit a 128 byte frame. The collecting is now one step function rather than a loop, because the NFC reader drives it from a poller loop and the BLE readers from notifications. All three use it, and the credential comes out through the shared parser that handles the long form. The native reader also took one trailing byte off each frame rather than the two byte status word. The collector does that in one place.
A command that expects data back carries a protected Le object and covers it with the checksum; one that does not, does neither. Wrapping always carried it, so a write named an object the standard says is not there and checksummed it. Our own tests passed because both ends of the same wrap agreed with each other, which is why the test for this compares the bytes rather than round tripping. Earlier in this work I removed this fix on the strength of a capture. The capture only showed that our wrap verifies against itself. The specification's command tables and the reference implementation both condition the object on the command having an Le. GET RESPONSE ignored the length the reader asked for and returned as much as the frame would hold. It now returns at most what was asked, with zero meaning the largest a single byte can state, and the four call sites check the command is long enough to carry that byte before reading it. An extended header list is tag and length pairs, not bare tags. It was walked as a tag list, which happened to work for the lists the tests used and would have misread a real one. A length asking for part of an object is refused rather than answered with the whole thing.
A command arriving in chained blocks was dropped with no reply at all, so the reader waited for an answer that was never coming. Nothing here needs to assemble one, but it is now refused with an R-block carrying the block number it was sent, which is something the other end can act on. The decrypted select answer and the diversifier taken out of it were both written to the log. That is the material a card's keys are derived from, and it does not belong on a console. The flag saying a handler had already written its own status word was file scope, which only worked because one listener runs at a time. It sits on the emulator now. Finishes a docstring that stopped mid-sentence.
Both reviewers suggested replacing the per-message scratch with one buffer per session, to stop a long exchange fragmenting the heap. Sizing the scratch from the message is what removed a fixed ceiling and about 850 bytes of per-call stack, so it is worth knowing the number before undoing it. Two thousand exchanges through one session: eighteen allocations each, and every byte given back. Several of those are the buffer mock taking two allocations where the device takes one, so eighteen is an upper bound on churn rather than the device's figure. Nothing is retained, so this is churn and not a leak. Whether that much churn fragments anything is a question about the device's allocator, which a test on a workstation cannot answer. The number is pinned so a change that grows it shows up. The host build counts allocations through the furi shim, which the device never sees.
Each of these is a fix that landed on some transports and not others. An R-block refusing a chained command carried the card identifier bit across from the block it was refusing but sent no identifier byte after it. A block that claims one and omits it is malformed, and a reader that addressed this card with an identifier could not recover from it. The ordinary reply already appended the byte; the refusal now does too, and the test ties the bit to the header length so the two cannot drift again. The NFC emulator assigned a new session over a live pointer. The three BLE paths free first; this one did not, so authenticating twice in one field leaked the old session including the keys that freeing it clears. The reader had the same shape on a retry with another keyset. The BLE central answered a failed authenticate with silence and moved on to expect a read, which then ran with no session. Silence tells the other end its key was wrong; the other three answer with well formed nonsense instead, and the phase stays where it is. Also drops a stale description of how header lists are parsed, four lines of captured hex above a comment that says the same thing in words, and the note in the allocation test about where the suggestion came from.
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.
Still needs some more testing, but caught a bunch of edge cases and fixed them. Added a bunch of tests.
Reduced memory usage, avoided a bunch of code duplication. Trying to get memory pressure down.
Added a bunch of defensive checking on lengths. Still only one ADF. Will fix that later.