Socket ether linux step2 #17926

devnexen · 2025-02-25T12:45:56Z

No description provided.

ext/sockets/sockets.c

ext/sockets/sockets.stub.php

bukka · 2025-03-03T18:29:33Z

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 · 2025-03-15T23:34:19Z

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

ext/sockets/sockets.stub.php

ext/sockets/sockets.c

ext/sockets/sockets.stub.php

ext/sockets/sockets.c

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 · 2025-06-30T16:14:13Z

ext/sockets/sockets.c

-			ZSTR_LEN(recv_buf) = retval;
-			ZSTR_VAL(recv_buf)[ZSTR_LEN(recv_buf)] = '\0';
+
+			if (UNEXPECTED(slen < ETH_HLEN)) {


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

arnaud-lb · 2025-06-30T16:17:54Z

ext/sockets/sockets.c


-			retval = recvfrom(php_sock->bsd_socket, ZSTR_VAL(recv_buf), arg3, arg4, (struct sockaddr *)&sll, (socklen_t *)&slen);
+			retval = recvfrom(php_sock->bsd_socket, ZSTR_VAL(recv_buf), recv_len, recv_flags, (struct sockaddr *)&sll, (socklen_t *)&slen);


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 · 2025-06-30T16:39:15Z

ext/sockets/sockets.c


-			retval = recvfrom(php_sock->bsd_socket, ZSTR_VAL(recv_buf), arg3, arg4, (struct sockaddr *)&sll, (socklen_t *)&slen);
+			retval = recvfrom(php_sock->bsd_socket, ZSTR_VAL(recv_buf), recv_len, recv_flags, (struct sockaddr *)&sll, (socklen_t *)&slen);


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 · 2025-06-30T17:01:20Z

ext/sockets/sockets.c

@@ -1416,6 +1435,57 @@ PHP_FUNCTION(socket_bind)
 }
 /* }}} */

+#ifdef AF_PACKET
+#define ETH_SUB_CHECKLENGTH(a, lyr)						\


This macro is unused

arnaud-lb · 2025-06-30T17:24:54Z

ext/sockets/sockets.c

+	if ((char *)ipdata + sizeof(tcp) < ZSTR_VAL(recv_buf) + slen)
+		return FAILURE;


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().

Back to the drawing board due to UAF with previous version. This reverts commit cc11606.

arnaud-lb · 2025-07-02T13:50:38Z

ext/sockets/sockets.c

+			if (recv_flags > 0 && !(recv_flags & (MSG_PEEK|MSG_DONTWAIT|MSG_ERRQUEUE))) {
+				zend_argument_value_error(4, "must set one the flags MSG_PEEK, MSG_DONTWAIT, MSG_ERRQUEUE");
+				zend_string_efree(recv_buf);
+				RETURN_THROWS();
+			}


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 · 2025-07-02T13:53:58Z

ext/sockets/sockets.c

+
+			switch (protocol) {
+				case ETH_P_IP: {
+					if (php_socket_get_chunk(&ether_hdr_buf, &raw_buf, 0, sizeof(struct iphdr)) == FAILURE) {


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

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

ext/sockets/tests/socket_afpacket.phpt

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 · 2025-07-06T09:04:12Z

ext/sockets/sockets.c

+					zend_update_property_string(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("dstMac"), ether_ntoa((struct ether_addr *)innere.h_dest));
+					zend_update_property_long(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("headerSize"), ETH_HLEN);
+					zend_update_property(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("socket"), socket);
+					zend_update_property_stringl(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("rawPacket"), (char *)ether_hdr_buf.buf, ether_hdr_buf.chunk_len);


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

arnaud-lb · 2025-07-06T09:05:17Z

ext/sockets/sockets.c

+			zval *addr = arg5;
+			zval *index = arg6;
+			if (recv_flags > 0 && (recv_flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_ERRQUEUE))) {
+				zend_argument_value_error(4, "must set one the flags MSG_PEEK, MSG_DONTWAIT, MSG_ERRQUEUE");


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");

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

arnaud-lb · 2025-07-06T09:12:00Z

ext/sockets/sockets.c

+					struct ipv6hdr ip;
+					memcpy(&ip, ether_hdr_buf.buf, sizeof(ip));
+					size_t totalip = sizeof(ip) + ip.payload_len;
+					if (totalip < slen) {


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 · 2025-07-06T09:12:50Z

ext/sockets/sockets.c

+						zend_string_efree(recv_buf);
+						zend_value_error("invalid transport header length");


Suggested change

zend_string_efree(recv_buf);

zend_value_error("invalid transport header length");

zend_value_error("invalid IPv6 payload len");

arnaud-lb · 2025-07-06T09:13:47Z

ext/sockets/sockets.c

+					zend_update_property_string(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("srcAddr"), inet_ntoa(s));
+					zend_update_property_string(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("dstAddr"), inet_ntoa(d));
+					zend_update_property_long(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("headerSize"), totalip);
+					zend_update_property_stringl(Z_OBJCE(zpayload), Z_OBJ(zpayload), ZEND_STRL("rawPacket"), (char *)ether_hdr_buf.buf, ether_hdr_buf.chunk_len);


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

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

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.

github-actions bot added Category: Build System Extension: sockets ABI break labels Feb 25, 2025

devnexen force-pushed the socket_ether_linux_step2 branch 2 times, most recently from e201885 to 1f1f204 Compare February 26, 2025 21:09

devnexen marked this pull request as ready for review February 26, 2025 21:46

devnexen requested a review from kocsismate as a code owner February 26, 2025 21:46

devnexen requested a review from arnaud-lb February 26, 2025 21:46

arnaud-lb reviewed Feb 27, 2025

View reviewed changes

devnexen marked this pull request as draft February 27, 2025 12:16

devnexen marked this pull request as ready for review February 27, 2025 21:28

devnexen marked this pull request as draft March 7, 2025 19:52

devnexen force-pushed the socket_ether_linux_step2 branch 2 times, most recently from 9e8daaa to 8cd42ca Compare March 7, 2025 22:29

devnexen marked this pull request as ready for review March 8, 2025 18:14

devnexen force-pushed the socket_ether_linux_step2 branch 2 times, most recently from c2fa112 to 3a67b50 Compare March 20, 2025 04:18

arnaud-lb reviewed Apr 7, 2025

View reviewed changes

devnexen force-pushed the socket_ether_linux_step2 branch 7 times, most recently from f8cb8fe to d23895d Compare April 13, 2025 13:05

devnexen requested a review from arnaud-lb April 13, 2025 13:45

arnaud-lb reviewed Apr 14, 2025

View reviewed changes

arnaud-lb reviewed Apr 15, 2025

View reviewed changes

ext/sockets/sockets.c Outdated Show resolved Hide resolved

devnexen force-pushed the socket_ether_linux_step2 branch 16 times, most recently from 7aed82a to 57066bc Compare June 30, 2025 11:04

arnaud-lb reviewed Jun 30, 2025

View reviewed changes

devnexen force-pushed the socket_ether_linux_step2 branch 2 times, most recently from a7b421f to 40850b2 Compare July 1, 2025 17:18

ext/sockets: follow-up on AF_PACKET support.

423bfb7

Back to the drawing board due to UAF with previous version. This reverts commit cc11606.

devnexen force-pushed the socket_ether_linux_step2 branch from 40850b2 to 423bfb7 Compare July 1, 2025 18:28

devnexen marked this pull request as ready for review July 1, 2025 19:26

devnexen requested a review from arnaud-lb July 1, 2025 19:26

arnaud-lb reviewed Jul 2, 2025

View reviewed changes

devnexen added 4 commits July 2, 2025 20:36

address code concerns

120cf76

start to address test changes

f72d78c

further tests changes

e939dda

tests...

a889885

devnexen requested a review from arnaud-lb July 2, 2025 21:34

arnaud-lb reviewed Jul 6, 2025

View reviewed changes


		retval = recvfrom(php_sock->bsd_socket, ZSTR_VAL(recv_buf), arg3, arg4, (struct sockaddr )&sll, (socklen_t )&slen);
		retval = recvfrom(php_sock->bsd_socket, ZSTR_VAL(recv_buf), recv_len, recv_flags, (struct sockaddr )&sll, (socklen_t )&slen);

		if ((char *)ipdata + sizeof(tcp) < ZSTR_VAL(recv_buf) + slen)
		return FAILURE;

	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");

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

		zend_string_efree(recv_buf);
		zend_value_error("invalid transport header length");

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

Socket ether linux step2 #17926

Are you sure you want to change the base?

Socket ether linux step2 #17926

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!