Third party security review

[azonenberg]

I'd like someone other than myself to look at the code at some point...

[taralx]

So far nothing looks wrong, but some comments:

1. When importing external code, it's a best practice to import as-is in one commit and then make changes to it in a separate one.
2. Some compilers can detect bounds issues if you use e.g. const uint8_t pub[ECDSA_KEY_SIZE] instead of const uint8_t* pub in parameter declarations. Might even be worth making it a wrapping struct that forms part of the API so you can rely on type checking.
3. https://github.com/azonenberg/staticnet/blob/2ffc36711bcd65cd90ff09108081eef18c1e8235/crypt/CryptoEngine.cpp#L160 this magic number concerns me. It's a footgun for future changes.
4. https://github.com/azonenberg/staticnet/blob/2ffc36711bcd65cd90ff09108081eef18c1e8235/crypt/CryptoEngine.cpp#L197 pretty sure you're supposed to use crypto_sign_keypair to get this. FWIW you do need the pk in EdDSA unless you want to waste time recalculating it on every signature.
5. https://github.com/azonenberg/staticnet/blob/2ffc36711bcd65cd90ff09108081eef18c1e8235/crypt/CryptoEngine.cpp#L203 another footgun perhaps? It's a good idea to compute these constants.
6. https://github.com/azonenberg/staticnet/blob/2ffc36711bcd65cd90ff09108081eef18c1e8235/drivers/stm32/STM32CryptoEngine.cpp#L170-L176 I'm very surprised to see this kind of thing without a critical section of some sort. Cooperative multitasking?
7. https://github.com/azonenberg/staticnet/blob/2ffc36711bcd65cd90ff09108081eef18c1e8235/drivers/stm32/STM32CryptoEngine.cpp#L227-L229 do you need a similar check in update?
8. https://github.com/azonenberg/staticnet/blob/2ffc36711bcd65cd90ff09108081eef18c1e8235/net/ipv4/IPv4Protocol.cpp#L294 if your gateway doesn't respond, does this mean you start spamming requests?

[azonenberg]

1. When did I do that? I generally try to keep import and modify separate.
2. Ooh, good call I should look into that.
3. Yeah I should probably add some more global macros or something for base64 coded key sizes. The CryptoEngine code as it stands now is pretty fixed on curve25519 though, a lot of stuff would have to change if I ever swap that out. Swapping AES for something else would be more straightforward.
4. Hmm, I think that comment is complaining about the memory usage of having to store the key in an argument but also copy it over the signed message? I'll look into that more. EDIT: crypto_sign_keypair generates a signing keypair, it doesn't sign a keypair, so not applicable here.
5. Yeah I should add more constants for that.
6. The stack is not intended to be thread safe since none of my stuff is RTOS based. You can have multiple open sockets but only one stack/sshd method can be in progress at any given time. The typical workflow is ISRs to handle packet reception then a single event loop in main() that pops Ethernet frames out of the input buffer and calls stack methods to process each one. I could perhaps document this more clearly, but I don't see this as being a concern in any of my deployments.
7. Same as above
8. Yes, every time we attempt to send a packet outside our subnet, if there's no gateway, we'll send an ARP query. The stack is incapable of initiating communication, though (it's an embedded server stack so all it does is respond to incoming requests from clients) so the number of packets sent depends on the number of requests made by off-subnet users. If those packets are reaching you the gateway must exist, so this is only really an issue if you're misconfigured with a bad gateway. This seems like the kind of thing that would get noticed and corrected fast since it renders the board unreachable.

[taralx]

• 1. The TweetNaCl import seems to be monolithic. Maybe I just missed the split?
• 4. Yeah, you are intended use crypto_sign_keypair to generate the concatenated sk/pk thing. But what you have works fine.
• 6. What happens if an ISR interrupts an Update? It'll swap out the state, but not swap it back in. That's why I was talking about critical sections.
• 7. Huh? Not sure why Finish needs to wait for busy but Update doesn't.

[azonenberg]

I don't have any interrupts that touch the crypto engine at all, the only concern would be if I had preemptive multitasking and you could swap in a separate thread also doing crypto before the CryptoEngine call had completed.

In my current platforms, that's not possible so there's no concern that I can see. If a UART or SPI bus ISR is called during any of the STM32CryptoEngine APIs we should just return right back to where we left off, unless I missed something?

The point of the context switching in STM32CryptoEngine is to allow multiple concurrent SSH sessions (which might be hashing data arriving in multiple TCP segments, with half-processed data in flight).

So the scenario is, call SHA256_Init(), SHA256_Update(), don't call final because we're waiting for more TCP data in the handshake, then another session needs to hash something. There's no thread safety issues because packets from the different sessions are processed sequentially in the single global event loop.

As far as waiting for the busy flag, see ST RM0468 section 41.7.2. Writes to HASH_DIN will block if a hash block is in progress. If I had critical hard realtime constraints that might be a problem since it would stall the AHB bus that other peripherals might need, but the MAC is in the FPGA and everything else has enough buffering it can wait.

The final HASH_BUSY check is possibly not even needed, but IIRC it was in the example code I used as a reference and I figured it didn't hurt to keep.

[taralx]

Ok, sounds good. Essentially a kind of cooperative multitasking. Might be worth a comment, but for the purposes of review it works fine.