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 · 2025-04-14T11:21:32Z

ext/sockets/sockets.c

+					struct ipv6hdr a;
+					memcpy(&a, payload, sizeof(a));
+					struct ipv6hdr *ip = &a;
+					size_t totalip = sizeof(*ip) + ip->payload_len;


We should not trust ip->payload_len

arnaud-lb · 2025-04-14T11:22:22Z

ext/sockets/sockets.c

+
+			switch (protocol) {
+				case ETH_P_IP: {
+					payload = ((unsigned char *)e + ETH_HLEN);


We must check that slen >= ETH_HLEN

arnaud-lb · 2025-04-14T11:24:41Z

ext/sockets/sockets.c

+					switch (ip->protocol) {
+						case IPPROTO_TCP: {
+							struct tcphdr a;
+							memcpy(&a, ipdata, sizeof(a));


We must check that ipdata + sizeof(tcphdr) < ZSTR_VAL(recv_buf) + slen

arnaud-lb · 2025-04-14T11:25:04Z

ext/sockets/sockets.c

+						}
+						case IPPROTO_UDP: {
+							struct udphdr a;
+							memcpy(&a, ipdata, sizeof(a));


arnaud-lb · 2025-04-14T11:25:18Z

ext/sockets/sockets.c

+					break;
+				}
+				case ETH_P_IPV6: {
+					payload = ((unsigned char *)e + ETH_HLEN);


arnaud-lb · 2025-04-14T11:28:06Z

ext/sockets/sockets.c

+					switch (ipprotocol) {
+						case IPPROTO_TCP: {
+							struct tcphdr a;
+							memcpy(&a, ipdata, sizeof(a));


Here too (I think this switch block is the same as in the ETH_P_IP case, and could be extracted to a separate function?)

arnaud-lb · 2025-04-14T11:28:59Z

ext/sockets/sockets.c

+						}
+						case IPPROTO_UDP: {
+							struct udphdr a;
+							memcpy(&a, ipdata, sizeof(a));


arnaud-lb · 2025-04-14T11:29:05Z

ext/sockets/sockets.c

+					break;
+				}
+				case ETH_P_LOOP: {
+					struct ethhdr *innere = (struct ethhdr *)((unsigned char *)e + ETH_HLEN);


arnaud-lb · 2025-04-15T09:20:24Z

ext/sockets/sockets.c

+	zend_update_property_long(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("srcPort"), ntohs(tcp->th_sport));
+	zend_update_property_long(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("dstPort"), ntohs(tcp->th_dport));
+	zend_update_property_long(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("headerSize"), sizeof(*tcp));
+	zend_update_property_string(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("rawPacket"), (char *)ipdata);


We should use zend_update_property_stringl() here (and in other places), as ipdata is not a null-terminated string

devnexen · 2025-04-26T20:17:25Z

@arnaud-lb I plan to add more tests later but let me know if, at least, the direction it is taking looks ok.

arnaud-lb · 2025-04-28T14:23:17Z

@devnexen yes, the direction it is taking is looking better! It feels safer now with php_socket_get_chunk(), and php_socket_afpacket_add_tcp() / php_socket_afpacket_add_udp() reduce code duplication.

arnaud-lb · 2025-04-28T14:12:33Z

ext/sockets/sockets.c

+		return FAILURE;
+	}
+
+	memcpy(ZSTR_VAL(dst), ZSTR_VAL(src) + offset, len);


Please also set the trailing null byte

arnaud-lb · 2025-05-19T09:32:48Z

ext/sockets/sockets.c

+	zend_update_property_long(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("srcPort"), ntohs(tcp->th_sport));
+	zend_update_property_long(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("dstPort"), ntohs(tcp->th_dport));
+	zend_update_property_long(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("headerSize"), sizeof(*tcp));
+	zend_update_property_stringl(Z_OBJCE_P(szpayload), Z_OBJ_P(szpayload), ZEND_STRL("rawPacket"), (char *)ipdata, sizeof(*tcp));


sizeof(*tcp) is the header size, but you want the full packet size here

arnaud-lb · 2025-05-19T09:34:25Z

ext/sockets/sockets.c

+	struct tcphdr a;
+	ETH_SUB_CHECKLENGTH(a, "TCP");
+	memcpy(&a, ipdata, sizeof(a));
+	struct tcphdr *tcp = &a;


Suggested change

struct tcphdr a;

ETH_SUB_CHECKLENGTH(a, "TCP");

memcpy(&a, ipdata, sizeof(a));

struct tcphdr *tcp = &a;

struct tcphdr tcp;

ETH_SUB_CHECKLENGTH(tcp, "TCP");

memcpy(&tcp, ipdata, sizeof(a));

Then use &tcp instead of tcp bellow.

arnaud-lb · 2025-05-19T09:35:33Z

ext/sockets/sockets.c

+static zend_result php_socket_afpacket_add_tcp(unsigned char *ipdata, struct sockaddr_ll sll, char *ifrname, zend_string *recv_buf,
+					       size_t slen, zval *szpayload,  zval *zpayload, zval *obj, zval *arg2, zval *arg5, zval *arg6) {


Most params here are unused. These could be removed.

arnaud-lb · 2025-05-19T09:35:38Z

ext/sockets/sockets.c

+static zend_result php_socket_afpacket_add_udp(unsigned char *ipdata, struct sockaddr_ll sll, char *ifrname, zend_string *recv_buf,
+					       size_t slen, zval *szpayload,  zval *zpayload, zval *obj, zval *arg2, zval *arg5, zval *arg6) {


Most params here are unused. These could be removed.

arnaud-lb · 2025-05-19T09:55:48Z

ext/sockets/sockets.c

+			struct ethhdr a;
+			memcpy(&a, ZSTR_VAL(dst_buf), ETH_HLEN);
+			struct ethhdr *e = &a;


Suggested change

struct ethhdr a;

memcpy(&a, ZSTR_VAL(dst_buf), ETH_HLEN);

struct ethhdr *e = &a;

struct ethhdr e;

memcpy(&e, ZSTR_VAL(dst_buf), ETH_HLEN);

arnaud-lb · 2025-05-19T09:58:48Z

ext/sockets/sockets.c

+					struct iphdr a;
+					memcpy(&a, payload, sizeof(a));
+					struct iphdr *ip = &a;


Suggested change

struct iphdr a;

memcpy(&a, payload, sizeof(a));

struct iphdr *ip = &a;

struct iphdr ip;

memcpy(&ip, payload, sizeof(ip));

arnaud-lb · 2025-05-19T10:00:16Z

ext/sockets/sockets.c

+					size_t tlayer = ip->ihl * 4;
+					size_t totalip = ntohs(ip->tot_len);
+
+					if (tlayer < sizeof(*ip) || totalip < tlayer || totalip < slen) {


We can probably replace this check by a php_socket_get_chunk() call now

arnaud-lb · 2025-05-19T10:01:34Z

ext/sockets/sockets.c

+					if (tlayer < sizeof(*ip) || totalip < tlayer || totalip < slen) {
+						ZVAL_NULL(&zpayload);
+						zend_update_property(Z_OBJCE(obj), Z_OBJ(obj), ZEND_STRL("payload"), &zpayload);
+						zend_update_property_string(Z_OBJCE(obj), Z_OBJ(obj), ZEND_STRL("rawPacket"), ZSTR_VAL(recv_buf));


rawPacket will be truncated if it contains null bytes. We should specify a length, or use zend_update_property_str() (in which case we should replace zend_string_efree() by zend_string_release() bellow).

arnaud-lb · 2025-05-19T10:04:16Z

ext/sockets/sockets.c

+					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 *)payload, totalip);


The length look wrong

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

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

devnexen marked this pull request as draft April 21, 2025 20:33

devnexen force-pushed the socket_ether_linux_step2 branch 3 times, most recently from ac62db0 to 6d9466a Compare April 26, 2025 19:06

devnexen force-pushed the socket_ether_linux_step2 branch from 696ef35 to 6b017cf Compare April 28, 2025 20:28

devnexen marked this pull request as ready for review April 28, 2025 21:59

devnexen requested a review from arnaud-lb May 3, 2025 20:39

arnaud-lb reviewed May 19, 2025

View reviewed changes

devnexen added 6 commits May 19, 2025 12:23

ext/sockets: follow-up on AF_PACKET support.

ef05c13

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

address most of remarks

4c77d90

fix unterminated buffers.

5708d64

strengthening the frame buffer handling

6b6bc1f

fix tests

bbe375c

add more tests

cdb5126

devnexen marked this pull request as draft May 19, 2025 11:37

address most of the remarks

83453b9

devnexen force-pushed the socket_ether_linux_step2 branch from 6b017cf to 83453b9 Compare May 19, 2025 11:37

fix buffer lengths and a test expectation

a66492e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket ether linux step2 #17926

Socket ether linux step2 #17926

devnexen commented Feb 25, 2025

bukka commented Mar 3, 2025 •

edited

Loading

devnexen commented Mar 15, 2025

arnaud-lb left a comment

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 14, 2025

arnaud-lb Apr 15, 2025

devnexen commented Apr 26, 2025

arnaud-lb commented Apr 28, 2025

arnaud-lb Apr 28, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

arnaud-lb May 19, 2025

		static zend_result php_socket_afpacket_add_tcp(unsigned char ipdata, struct sockaddr_ll sll, char ifrname, zend_string *recv_buf,
		size_t slen, zval szpayload, zval zpayload, zval obj, zval arg2, zval arg5, zval arg6) {

		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 *)payload, totalip);

Socket ether linux step2 #17926

Are you sure you want to change the base?

Socket ether linux step2 #17926

Conversation

devnexen commented Feb 25, 2025

bukka commented Mar 3, 2025 • edited Loading

devnexen commented Mar 15, 2025

arnaud-lb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen commented Apr 26, 2025

arnaud-lb commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka commented Mar 3, 2025 •

edited

Loading