What's going wrong with this SOCKS proxy request?

Question

I'm currently trying to implement SOCKS 4/5 functionality in my C++ program (i.e. requests to arbitrary protocols and hosts can be redirected through a given SOCKS proxy if desired). I'm developing purely for Windows so using Winsock 2.

My problem is slightly less abstract than simply "how does this work" though. I've read the RFC for SOCKS 4 (I decided to implement SOCKS 4 first since it has less bytes in its requests to contend with) but I'm struggling to create the C string I need to send().

At present, I have a struct defined called Socks4Msg which looks like this:

struct Socks4Msg {
const static uint8_t version = 0x04; //SOCKS version 4 (obviously)
const static uint8_t command = 0x01; //1 is TCP CONNECT command
const static uint8_t nullbyte = 0x00; //null byte sent at message end

uint16_t port; //16 bit/2 byte port (network order)
uint32_t ip; //32 bit/4 byte IP address (network order)
Socks4Msg(uint16_t p, uint32_t i) : port(p), ip(i) { }
};

The function which creates the actual socket and does the work is here (where p and h hold the port and host to test through the proxy -- p is a string to maintain compatibility with HttpProxy which I've already implemented). port and addr are part of the class and are an int and string respectively; they're the details of the proxy server.

int Socks4Proxy::test(std::string p, std::string h) const {
uint16_t network_port = htons(str_to_numt<uint16_t>(p));
uint32_t network_ip = hostname_to_ip(h);
Socks4Msg msg_struct(network_port,network_ip);

SOCKET s = socket(AF_INET,SOCK_STREAM,IPPROTO_TCP);
int last_error;
if(s == INVALID_SOCKET) {
              last_error = WSAGetLastError();
              std::cerr << "Failed to initialise socket! Error code: " << last_error << std::endl; 
              return 2;
}

sockaddr_in st_addr;
st_addr.sin_family = AF_INET;
st_addr.sin_port = htons(port);
ipaddr_t ip = inet_addr(addr.c_str());
st_addr.sin_addr.S_un.S_addr = ip;

if(connect(s,(sockaddr*)&st_addr,sizeof(st_addr))!=0) {
                                                      last_error = WSAGetLastError();
                                                      std::cerr << "Socket failed to connect. Error code: " << last_error << std::endl;
                                                      return 2;  
}

uint8_t message[13];
uint8_t* message_ptr;
memset(message, 0, 13);
message_ptr = message;
*message_ptr = msg_struct.version;
message_ptr++;
*message_ptr = msg_struct.command;
message_ptr++;
*message_ptr = msg_struct.port;
message_ptr += 2;
*message_ptr = msg_struct.ip;
message_ptr += 4;
*message_ptr = 'b'; message_ptr++; *message_ptr = 'o'; message_ptr++; *message_ptr = 'b'; message_ptr++;
*message_ptr = msg_struct.nullbyte;
message_ptr++;
*message_ptr = 0x00;
char smessage[13];
memcpy(smessage, message, 13);

int return_val;
while(return_val = send(s, smessage, strlen(smessage), 0)) {
                       if(return_val == SOCKET_ERROR) {
                                     last_error = WSAGetLastError();
                                     std::cerr << "Writing data failed. Error code: " << last_error << std::endl;
                                     return 2;
                       }
                       //implement return_val < strlen(message) here
                       else break;
}
//remainder of function

I have tested and verified that the members of msg_struct contain the correct data (and in the correct byte order) before the C string manipulation starts.

I've tried doing it using memcpy() (e.g. memcpy(message_ptr, &msg_struct.port, 2)) in place of the assignments but I just can't understand why Wireshack always quotes the byte length of the sent data as 2 (i.e. version and command) but nothing else. (I know my knowledge of C strings - and therefore the code at that point - is a bit rough but I can't explain why it doesn't work)

Any advice would be greatly appreciated!

Answer 1

First of all message_ptr is uint8_t* and *message_ptr = msg_struct.ip; is wrong. You should cast message_ptr to uint_32t* and then assign data, like * ((uint32_t*)message_ptr) = msg_struct.ip; otherwise msg_struct.ip will be converted to uint8_t and then assigned. Same problems with other fields.

Check this and let me know if it is woring again :)

BTW. I think Wireshark network traffic analyzer could help you a lot in searching such kind of problems.

UPDATE

Probably a better idea is to create a structure which represents the message you want to send and cast message_ptr to the pointer on this structure. But do not forget to tell your compiler not to add any paddings.

UPDATE 2

Network and host byte order.

Do not forget that you should change bytes order using hton, ntoh, htonl or ntohl functions.