Socket ether linux step2

[devnexen]

No description provided.

[bukka]

This has a bit of class design so it should be probably at least posted to internals (it's not like adding a constant or a simple funciton). I agree that there should be some namespacing as well possibly. Definitely not the current names that will easily result in BC break for appliations.

[devnexen]

ping @arnaud-lb I think I have, at least, addressed the majority of your remarks :).

[arnaud-lb]

Sorry for the delay, I missed you ping

[arnaud-lb]

I found no more issues apart from a few comments.

This feels safe as all accesses to the read buffer are guarded by php_socket_get_chunk(). However, we often use this function only for checking the length, and we copy the buffer again after calling it. Maybe the function could be changed to something like this, instead:

typedef struct _php_socket_chunk {
    const char *c;
    size_t chunk_len; /* length of this chunk */
    size_t buf_len; /* length of this chunk plus everything after the chunk in c */
} php_socket_chunk;

static zend_result php_socket_get_chunk(php_socket_chunk *chunk, const php_socket_chunk *src, size_t offset, size_t len) {
	if (UNEXPECTED((offset > SIZE_MAX - len) || offset + len > src->buf_len)) {
		return FAILURE;
	}

        dest->c = src->c + offset;
        dest->chunk_len = len;
        dest->buf_len = src->buf_len - offset;
	return SUCCESS;
}

php_socket_chunk *chunk = {
    .s = ZSTR_VAL(recv_buf),
    .buf_len = ZSTR_LEN(recv_buf),
};

if (php_socket_get_chunk(chunk, chunk, 0, ETH_HLEN) == FAILURE) {
    ...
}

struct ethhdr e;
memcpy(&e, ethhdr_chunk->c, ETH_HLEN);

if (php_socket_get_chunk(chunk, chunk, 0, sizeof(struct iphdr)) == FAILURE) {
    ...
}

struct iphdr ip;
memcpy(&ip, ethhdr_chunk->c, sizeof(ip));

[arnaud-lb]

This could be replaced by a ZEND_ASSERT(), as both slen and ETH_HLEN are compile time constants.

[arnaud-lb]

With recv_flags & MSG_TRUNC, it is possible that retval is longer than ZSTR_LEN(recv_buf).

I suggest that we allow only a finite set of known and supported flags in recv_flags.

[arnaud-lb]

Also, ZSTR_LEN(recv_buf) may be larger than retval. You need to update ZSTR_LEN(recv_buf) after this call, otherwise you expose uninitialized memory past the packet data:

$s_c     = socket_create(AF_PACKET, SOCK_RAW, ETH_P_ALL);
$s_bind  = socket_bind($s_c, 'lo');
$s_s     = socket_create(AF_PACKET, SOCK_RAW, ETH_P_LOOP);
$v_bind  = socket_bind($s_s, 'lo');

$buf = pack("H12H12n", "ffffffffffff", "000000000000", ETH_P_LOOP);
$buf .= str_repeat("A", 46);

printf("buf len: %d\n", strlen($buf));

socket_sendto($s_s, $buf, strlen($buf), 0, "lo", 1);
socket_recvfrom($s_c, $rsp, strlen($buf)+1000, 0, $addr);

var_dump(bin2hex($rsp->rawPacket));

[arnaud-lb]

This macro is unused

[arnaud-lb]

I don't understand this check. Is it necessary, when the length of ipdata is already checked by php_socket_get_chunk()? Same remark for php_socket_afpacket_add_udp().

[arnaud-lb]

This allows to pass any flag as long as recv_flags contains one of MSG_PEEK|MSG_DONTWAIT|MSG_ERRQUEUE. You probably want something like if (recv_flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_ERRQUEUE)) { error }.

[arnaud-lb]

I'm not sure, but I think you wanted to set offset to ETH_HLEN here.

[devnexen]

yes that explain and the code typo issue below some tests inconsistencies

[arnaud-lb]

I'm starting to question the idea of parsing packets in C. It is very easy to make mistakes here, which would result in security issues, specially as AF_PACKET requires elevated privileges.

In a more security-conscious approach we would return the recvfrom() buffer as a string, and let userland parse it safely.

[arnaud-lb]

Should be .buf_len, not .chunk_len, otherwise this exposes only the header, not the raw packet

[arnaud-lb]

Suggested change

	zend_argument_value_error(4, "must set one the flags MSG_PEEK, MSG_DONTWAIT, MSG_ERRQUEUE");
	zend_argument_value_error(4, "contains invalid flags. It must be zero or more of MSG_PEEK, MSG_DONTWAIT, MSG_ERRQUEUE");

[devnexen]

I think the first part is redundant, the exception already mentions the argument name which is $flags.

[arnaud-lb]

We should not use slen (the size of struct sockaddr_ll) here, and I think the comparison is inversed:

Suggested change

	if (totalip < slen) {
	size_t totalip = sizeof(ip) + ip.payload_len;
	if (totalip > ether_hdr_buf.buf_len) {

[arnaud-lb]

Suggested change

	zend_string_efree(recv_buf);
	zend_value_error("invalid transport header length");
	zend_value_error("invalid IPv6 payload len");

[arnaud-lb]

ether_hdr_buf is not the raw packet, as it starts after the IP header

[devnexen]

so is the whole raw buffer from start ? if yes we already store it in the "root" part.

[arnaud-lb]

The raw IP packet starts at the beginning of the IP header. You call php_socket_get_chunk() two times in case ETH_P_IP:: The first one gives you a chunk that starts at the right position for rawPacket. You probably need a separate chunk variable for the second call.

[arnaud-lb]

I don't find new problems, but I think this needs an additional review from someone else.

Also, I still think that doing the parsing in userland would make sense, because of the security implications. recvfrom() could expose only the raw recv buffer as a string, and a userland package could handle the parsing.

[devnexen]

Fair enough and thanks for your time :) I may ask @nielsdos then if there is enough push back I m not too stubborn I ll give up on this idea at least I learnt a lot doing it :)