A key inspiration for this post is Sam Reich, who lives in my general neighborhood.
Reich has spent seven seasons of the show "Game Changer" (Wikipedia) reminding everyone of a single crucial detail:
Sam: thanks for the reminder that there's always some troll hanging out. 😅 The current tech climate often focuses on uncovering vulnerabilities. So, in that spirit, let's dive in.
And so this tale will surely end well, won't it?
It all starts with me continuing to tinker with OpenWrt. More recently, I realized that extending the startup scripts for hev-socks5-server was a worthwhile endeavor (also, thanks Hev for this wonderful rabbit hole. If we're ever in the same room the drinks are on me).
See, I want to expose various SOCKS5 proxies for... reasons. I don't really want to get into it, and you'll notice that generally other people don't either (proxy-1, proxy-2). Deal with it.
Regardless, everything was looking good. I noticed an odd issue... I told it to
bind to a single interface and it bound to multiple interfaces—about four, in
fact—when I specified only a single device name. It turns out that it was just
me misunderstanding the difference between listen-address and bind-address.
In the course of that exploration, though, I went down an unexpected rabbit hole. I ended up (for god knows what reason) staring at some of the PPP code (as any self-respecting telco nerd would). Something didn't sit right with me. I couldn't exactly tell "why" but something felt fundamentally wrong with the code's behavior.
Then I remembered that modern tools can help. (Living in California, I'm no stranger to investing in tech, perhaps even overpaying for my energy guilt!) So, instead of just staring like Philip J. Fry at the source code... I could use ollama to introspect the code with the help of Aider. After some focused interaction with our long necked oracle, behold: the outcome...
A write up of a buffer overflow that's been lurking for 15+ years in lwIP. I don't want to steal anyone else's valor, but after not seeing a patch submitted upstream I went ahead and submitted it upstream. For anyone else working with PPP and wants it now either check out the write-up or grab the patch here:
From 6ee35e0fa368e3bfbafbc029ce0cd852e03168eb Mon Sep 17 00:00:00 2001
From: Brian 'redbeard' Harrington <redbeard@dead-city.org>
Date: Thu, 8 May 2025 19:41:09 -0700
Subject: [PATCH] fix(ppp): prevent heap-buffer-overflow in pppcrypt_get_7bits
The pppcrypt_get_7bits function could read one byte beyond the input
buffer when processing the last bits of a 56-bit (7-byte) key. This
occurred because it unconditionally accessed `input[startBit / 8 + 1]`
without checking against the actual length of the input buffer.
This patch modifies pppcrypt_get_7bits to accept the input buffer's
length and performs bounds checks before accessing bytes from the input.
If an access to the second byte (for constructing a 16-bit word) would
be out of bounds, a safe default value of 0 is used. The
pppcrypt_56_to_64_bit_key function has been updated to pass the correct
key length (7 bytes) to the modified pppcrypt_get_7bits.
---
src/netif/ppp/pppcrypt.c | 74 +++++++++++++++++++++++++++++++---------
1 file changed, 58 insertions(+), 16 deletions(-)
diff --git a/src/netif/ppp/pppcrypt.c b/src/netif/ppp/pppcrypt.c
index 82d78c13..9a457101 100644
--- a/src/netif/ppp/pppcrypt.c
+++ b/src/netif/ppp/pppcrypt.c
@@ -38,29 +38,71 @@
#include "netif/ppp/pppcrypt.h"
-static u_char pppcrypt_get_7bits(u_char *input, int startBit) {
+static u_char pppcrypt_get_7bits(u_char *input, int startBit, int input_len_bytes) {
unsigned int word;
+ u_char byte1_val;
+ u_char byte2_val = 0; /* Initialize to 0 for safety */
+ int byte1_idx = startBit / 8;
- word = (unsigned)input[startBit / 8] << 8;
- word |= (unsigned)input[startBit / 8 + 1];
+ /* Check if byte1_idx is within bounds */
+ if (byte1_idx < input_len_bytes) {
+ byte1_val = input[byte1_idx];
+ } else {
+ /* This case should ideally not be reached if startBit is always valid
+ * for the start of a 7-bit sequence within a 56-bit key.
+ * However, as a safeguard, returning a value that will be masked
+ * or handled as an error might be appropriate.
+ * For DES key generation, this path implies a logic error in the caller
+ * if input_len_bytes is correct for the key type.
+ * Returning 0xFE ensures the parity bit is 0 after masking,
+ * consistent with the function's behavior.
+ */
+ return 0xFE;
+ }
- word >>= 15 - (startBit % 8 + 7);
+ /* Check if byte1_idx + 1 is within bounds for byte2_val */
+ if (byte1_idx + 1 < input_len_bytes) {
+ byte2_val = input[byte1_idx + 1];
+ }
+ /* If byte1_idx + 1 is out of bounds, byte2_val remains 0,
+ * which is safe for the bitwise operations below as it won't contribute
+ * bits from an invalid memory location.
+ */
- return word & 0xFE;
+ word = (unsigned int)byte1_val << 8;
+ word |= (unsigned int)byte2_val;
+
+ /* Shift to align the 8 bits (of which 7 are significant) starting at startBit.
+ The (startBit % 8) gives the bit offset within the first byte (byte1_val).
+ We want 7 bits from this stream, and the 8th bit is also part of the shift logic.
+ The expression (15 - ((startBit % 8) + 7)) calculates how many bits to shift
+ the 16-bit `word` to the right so that the desired 8-bit segment (containing
+ the 7 significant bits and one more) is at the LSB position.
+ Example: startBit = 0. Shift = 15 - (0 + 7) = 8. (B1<<8 | B2) >> 8 = B1.
+ Example: startBit = 1. Shift = 15 - (1 + 7) = 7. (B1<<8 | B2) >> 7.
+ Example: startBit = 7. Shift = 15 - (7 + 7) = 1. (B1<<8 | B2) >> 1.
+ */
+ word >>= (15 - ((startBit % 8) + 7));
+
+ return (u_char)(word & 0xFE); /* Return the most significant 7 bits of the 8-bit segment, with LSB (parity) zeroed */
}
-/* IN 56 bit DES key missing parity bits
- * OUT 64 bit DES key with parity bits added
+/* IN 56 bit DES key missing parity bits (7 bytes long)
+ * OUT 64 bit DES key with parity bits added (8 bytes long)
+ * (Parity bits are zeroed by pppcrypt_get_7bits, to be set later by a parity generation function)
*/
-void pppcrypt_56_to_64_bit_key(u_char *key, u_char * des_key) {
- des_key[0] = pppcrypt_get_7bits(key, 0);
- des_key[1] = pppcrypt_get_7bits(key, 7);
- des_key[2] = pppcrypt_get_7bits(key, 14);
- des_key[3] = pppcrypt_get_7bits(key, 21);
- des_key[4] = pppcrypt_get_7bits(key, 28);
- des_key[5] = pppcrypt_get_7bits(key, 35);
- des_key[6] = pppcrypt_get_7bits(key, 42);
- des_key[7] = pppcrypt_get_7bits(key, 49);
+void pppcrypt_56_to_64_bit_key(u_char *key, u_char *des_key) {
+ /* The input key is 56 bits, which is 7 bytes. */
+ const int key_input_len_bytes = 7;
+
+ des_key[0] = pppcrypt_get_7bits(key, 0, key_input_len_bytes);
+ des_key[1] = pppcrypt_get_7bits(key, 7, key_input_len_bytes);
+ des_key[2] = pppcrypt_get_7bits(key, 14, key_input_len_bytes);
+ des_key[3] = pppcrypt_get_7bits(key, 21, key_input_len_bytes);
+ des_key[4] = pppcrypt_get_7bits(key, 28, key_input_len_bytes);
+ des_key[5] = pppcrypt_get_7bits(key, 35, key_input_len_bytes);
+ des_key[6] = pppcrypt_get_7bits(key, 42, key_input_len_bytes);
+ des_key[7] = pppcrypt_get_7bits(key, 49, key_input_len_bytes);
}
#endif /* PPP_SUPPORT && MSCHAP_SUPPORT */
--
2.48.1