I'm trying to figure out if this could somehow be overflowed:
void print_address(char *p)
{
arp_ hw;
int i;
hw.length = (size) *(p + _OFFSET1); //189 + 4 = 193
memcpy(hw.addr, packet + _OFFSET2, hw.length);
return;
}
where packet is an input read from a .txt file?
hwaddr.len is an unsigned char which has range 0 to 255. So an attacker could send you a packet which declares length 255. Since hwaddr.addr is declared as a 128-byte buffer, the attacker can then deliver a payload of 127 bytes. Is that enough?
The usual x86 calling convention is to push the return address, push arguments, and then jump, at which point the callee will allocate each variable in the order declared. So, counting from the start of hwaddr, hwaddr.len will be 128 bytes above the stack pointer, packet will be 129 bytes above, and the return address will be 129 + sizeof(char *), which is at most 137 bytes even on a 64-bit system. So, yes, the attacker can overwrite your return address and deliver 118 bytes of shell code in addition.
Edit I just figured out the OP's confusion. When you encode the length as an unsigned char, this does not mean you use ASCII to represent the length. That is, you do not read this byte, call atoi() on it, and get a single-digit number ranging from 0 to 9. You just use the eight bits like a really narrow int type, where each bit represents a binary digit.
Yes, it can be overflowed; if the value at offset 4 in the packet is greater than 128, you will overflow the addr field in hwaddr.
Yes.
For example, if the line starts with "nnnn\xff" this will smash the stack.
Absolutely. You never even check that the input buffer is large enough.
The main thing that jumps out at me is this:
#define _LENGTH 128
...
typedef struct{
char addr[_LENGTH];
...
Then later on:
hwaddr.len = (shsize_t) *(packet + _OFFSET1); //189 + 4 = 193
memcpy(hwaddr.addr, packet + _OFFSET2, hwaddr.len);
This seems dangerous. You've only allocated 128 bytes for addr and haven't checked to verify that the length of information being copied in is <= _LENGTH.
You may want to dynamically allocate this when you know the length of the data coming in (if possible) or check to ensure you're not copying extra data into the addr char array:
if (hwaddr.len <= _LENGTH) {
memcpy(...);
}
Let's see... You take in a packet as an array of characters in which the fourth character in is the size the packet. Characters are 8 bit so when used as an unsigned numeral is covers the values 0-255. Your buffer length is set at 128. This will overflow, you need to do some sort of check on _LENGTH.
