{"thread":{"id":"59939","subject":"[PATCH] pkt-line: don't check string length in packet_length()","startedAt":"2023-07-01T07:11:51Z","lastAt":"2023-07-07T21:48:09Z","messageCount":8,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"479082","messageId":"89d58db7-6a01-b3fa-54f0-19d5a3819eb3@web.de","threadId":"59939","inReplyTo":null,"subject":"[PATCH] pkt-line: don't check string length in packet_length()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-01T07:05:15Z","receivedAt":"2023-07-01T07:11:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"hex2chr() takes care not to run over the end of a short string.\n101736a14c (pkt-line: extern packet_length(), 2020-05-19) turned the\ninput parameter of packet_length() from a string pointer into an array\nof known length, making string length checks unnecessary.  Get rid of\nthem by using hexval() directly.\n\nThe resulting branchless code is simpler and it becomes easier to see\nthat the function mirrors set_packet_header().\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n pkt-line.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 62b4208b66..6e022029ca 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -375,8 +375,10 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n\n int packet_length(const char lenbuf_hex[4])\n {\n-\tint val = hex2chr(lenbuf_hex);\n-\treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n+\treturn\thexval(lenbuf_hex[0]) << 12 |\n+\t\thexval(lenbuf_hex[1]) <<  8 |\n+\t\thexval(lenbuf_hex[2]) <<  4 |\n+\t\thexval(lenbuf_hex[3]);\n }\n\n static char *find_packfile_uri_path(const char *buffer)\n--\n2.41.0\n"},{"id":"479181","messageId":"xmqqttui3nqn.fsf@gitster.g","threadId":"59939","inReplyTo":"89d58db7-6a01-b3fa-54f0-19d5a3819eb3@web.de","subject":"Re: [PATCH] pkt-line: don't check string length in packet_length()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T05:56:32Z","receivedAt":"2023-07-05T05:56:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> hex2chr() takes care not to run over the end of a short string.\n> 101736a14c (pkt-line: extern packet_length(), 2020-05-19) turned the\n> input parameter of packet_length() from a string pointer into an array\n> of known length, making string length checks unnecessary.  Get rid of\n> them by using hexval() directly.\n\nI am puzzled about the part of the above description on \"making\nstring length checks unnecessary\".  The two callers we currently\nhave both do pass char[4], but the compiler would not stop us from\npassing a pointer to a memory region of an unknown size; if we\nbutcher one of the current callers\n\ndiff --git c/pkt-line.c w/pkt-line.c\nindex 6e022029ca..e1c49baefd 100644\n--- c/pkt-line.c\n+++ w/pkt-line.c\n@@ -421,7 +421,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\treturn PACKET_READ_EOF;\n \t}\n \n-\tlen = packet_length(linelen);\n+\tlen = packet_length(buffer);\n \n \tif (len < 0) {\n \t\tif (options & PACKET_READ_GENTLE_ON_READ_ERROR)\n\nwhere \"buffer\" is just a random piece of memory passed to the caller\nand there is no such guarantee like \"it at least is 4 bytes long\",\nwe would just slurp garbage and run past the end of the buffer.\n\n> The resulting branchless code is simpler and it becomes easier to see\n> that the function mirrors set_packet_header().\n\nI do like the resulting code, but I feel a bit uneasy to sell this\nchange as \"the code becomes more streamlined without losing safety\".\nIt looks more like \"this change is safe for our two callers; those\nadding more callers in the future are better be very careful\", no?\n"},{"id":"479199","messageId":"32f41065-a78c-aa60-0d78-4dbfa8827b1a@web.de","threadId":"59939","inReplyTo":"xmqqttui3nqn.fsf@gitster.g","subject":"Re: [PATCH] pkt-line: don't check string length in packet_length()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-05T16:15:56Z","receivedAt":"2023-07-05T16:16:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.07.23 um 07:56 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> hex2chr() takes care not to run over the end of a short string.\n>> 101736a14c (pkt-line: extern packet_length(), 2020-05-19) turned the\n>> input parameter of packet_length() from a string pointer into an array\n>> of known length, making string length checks unnecessary.  Get rid of\n>> them by using hexval() directly.\n>\n> I am puzzled about the part of the above description on \"making\n> string length checks unnecessary\".  The two callers we currently\n> have both do pass char[4], but the compiler would not stop us from\n> passing a pointer to a memory region of an unknown size; if we\n> butcher one of the current callers\n>\n> diff --git c/pkt-line.c w/pkt-line.c\n> index 6e022029ca..e1c49baefd 100644\n> --- c/pkt-line.c\n> +++ w/pkt-line.c\n> @@ -421,7 +421,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n>  \t\treturn PACKET_READ_EOF;\n>  \t}\n>\n> -\tlen = packet_length(linelen);\n> +\tlen = packet_length(buffer);\n>\n>  \tif (len < 0) {\n>  \t\tif (options & PACKET_READ_GENTLE_ON_READ_ERROR)\n>\n> where \"buffer\" is just a random piece of memory passed to the caller\n> and there is no such guarantee like \"it at least is 4 bytes long\",\n> we would just slurp garbage and run past the end of the buffer.\n\nIndeed, GCC warns if you give it a short array, but not if you pass a\npointer: https://godbolt.org/z/T3xfE5W9n.  Clang doesn't care at all.\n\nSo that was wishful thinking on my part.  Sorry.  :-/\n\n>> The resulting branchless code is simpler and it becomes easier to see\n>> that the function mirrors set_packet_header().\n>\n> I do like the resulting code, but I feel a bit uneasy to sell this\n> change as \"the code becomes more streamlined without losing safety\".\n> It looks more like \"this change is safe for our two callers; those\n> adding more callers in the future are better be very careful\", no?\n\nWith no way to enforce passing an array of a certain size to a function\nthe only safe options I see are keeping the length check, using a macro\nor inlining the calculation.  Hmm.\n\nRené\n"},{"id":"479243","messageId":"xmqq4jmiyu7s.fsf@gitster.g","threadId":"59939","inReplyTo":"32f41065-a78c-aa60-0d78-4dbfa8827b1a@web.de","subject":"Re: [PATCH] pkt-line: don't check string length in packet_length()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T20:33:11Z","receivedAt":"2023-07-05T20:33:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> I do like the resulting code, but I feel a bit uneasy to sell this\n>> change as \"the code becomes more streamlined without losing safety\".\n>> It looks more like \"this change is safe for our two callers; those\n>> adding more callers in the future are better be very careful\", no?\n>\n> With no way to enforce passing an array of a certain size to a function\n> the only safe options I see are keeping the length check, using a macro\n> or inlining the calculation.  Hmm.\n\nWe keep repeating \"length check\" because that is what the comment in\nthe function says, but even if the caller has 4-byte, that 4-byte\nsubstring at the beginning is what it read from the untrusted side\nover the network.  We should be checking if we have 4 hexadecimal\nlength even if we are not running beyond the end of the buffer, no?\n\nSo it may be that the comment needs to be fixed more than the code.\n\nThanks.\n"},{"id":"479244","messageId":"6f984150-5a98-b2b8-1791-b101f5953b7a@web.de","threadId":"59939","inReplyTo":"xmqq4jmiyu7s.fsf@gitster.g","subject":"Re: [PATCH] pkt-line: don't check string length in packet_length()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-05T21:11:15Z","receivedAt":"2023-07-05T21:11:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.07.23 um 22:33 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>>> I do like the resulting code, but I feel a bit uneasy to sell this\n>>> change as \"the code becomes more streamlined without losing safety\".\n>>> It looks more like \"this change is safe for our two callers; those\n>>> adding more callers in the future are better be very careful\", no?\n>>\n>> With no way to enforce passing an array of a certain size to a function\n>> the only safe options I see are keeping the length check, using a macro\n>> or inlining the calculation.  Hmm.\n\nThree more ideas: Wrap the buffer in a custom struct, pass the four\nbytes as a uint32_t or as four separate char parameters.  I better\nstop now..\n\n> We keep repeating \"length check\" because that is what the comment in\n> the function says, but even if the caller has 4-byte, that 4-byte\n> substring at the beginning is what it read from the untrusted side\n> over the network.  We should be checking if we have 4 hexadecimal\n> length even if we are not running beyond the end of the buffer, no?\n\nSure, that's done.  If any of the four characters is not a hexadecimal\ndigit then packet_length() returns a negative value, before and after\nthe change.\n\n> So it may be that the comment needs to be fixed more than the code.\n\nWhich comment specifically?  The one in pkt-line.h doesn't mention\nwhat happens if you pass in a short buffer -- leaving it undefined is\nOK, I think.  And in and around pkt-line.c::packet_length() I don't\nsee any comment?\n\nRené\n"},{"id":"479249","messageId":"xmqqsfa2uh83.fsf@gitster.g","threadId":"59939","inReplyTo":"6f984150-5a98-b2b8-1791-b101f5953b7a@web.de","subject":"Re: [PATCH] pkt-line: don't check string length in packet_length()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T22:27:24Z","receivedAt":"2023-07-05T22:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Sure, that's done.  If any of the four characters is not a hexadecimal\n> digit then packet_length() returns a negative value, before and after\n> the change.\n\nAh, it is the beauty of hexval[] table ;-)\n\nSo as long as we are sure that we are not running beyond the end of\nthe buffer, we are OK.  And as I already said, I think \"this change\nis safe for our two callers; those adding more callers in the future\nare better be very careful\" is probably good enough for this one.\n\nhex.h:hex2chr() says \"don't run over the end of short strings\", but\nas far as I can see it does not check any such thing; find a page of\nmemory, whose next page is unmapped, and pointing *s at the last\nbyte of that page and calling it will happily run over the end and\nwould cause SIGBUS.  The function assumes that such a short string\nis always NUL terminated, which is not a great way to guarantee that\nwe do not run over the end of strings.\n"},{"id":"479255","messageId":"a5acfab5-0133-b3d6-3efc-c3f107e78b58@web.de","threadId":"59939","inReplyTo":"xmqqsfa2uh83.fsf@gitster.g","subject":"Re: [PATCH] pkt-line: don't check string length in packet_length()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-06T05:07:43Z","receivedAt":"2023-07-06T05:08:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.07.23 um 00:27 schrieb Junio C Hamano:\n> hex.h:hex2chr() says \"don't run over the end of short strings\", but\n> as far as I can see it does not check any such thing; find a page of\n> memory, whose next page is unmapped, and pointing *s at the last\n> byte of that page and calling it will happily run over the end and\n> would cause SIGBUS.  The function assumes that such a short string\n> is always NUL terminated, which is not a great way to guarantee that\n> we do not run over the end of strings.\n\nYes, hex2chr() works with C strings, i.e. those that end with a NUL\ncharacter.  An empty string is just a NUL byte, a string of length 1\nis a non-NUL byte and a NUL.  The function reads one byte from the\nformer and otherwise two bytes -- no overrun.\n\nIf a C string loses its NUL, how could you detect its end?\n\nRené\n"},{"id":"479296","messageId":"4d2bb49b-9c3c-ea58-274b-d9e9586620c1@web.de","threadId":"59939","inReplyTo":"89d58db7-6a01-b3fa-54f0-19d5a3819eb3@web.de","subject":"[PATCH v2] pkt-line: add size parameter to packet_length()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-07T21:47:55Z","receivedAt":"2023-07-07T21:48:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"hex2chr() takes care not to run over the end of a NUL-terminated string.\nIt's used in packet_length(), but both callers of that function pass a\nfour-byte buffer, making NUL-checks unnecessary.  packet_length() could\naccidentally be used with a pointer to a buffer of unknown size at new\ncall-sites, though, and the compiler wouldn't complain.\n\nAdd a size parameter plus check, and remove the NUL-checks by calling\nhexval() directly.  This trades three NUL checks against one size check\nand the ability to report the use of a short buffer at runtime.\n\nIf any of the four bytes is NUL or -- more generally -- not a\nhexadecimal digit, then packet_length() still returns a negative value.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n pkt-line.c    | 12 ++++++++----\n pkt-line.h    |  2 +-\n remote-curl.c |  3 ++-\n 3 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 62b4208b66..fcfa357ccd 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -373,10 +373,14 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n\n-int packet_length(const char lenbuf_hex[4])\n+int packet_length(const char lenbuf_hex[4], size_t size)\n {\n-\tint val = hex2chr(lenbuf_hex);\n-\treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n+\tif (size < 4)\n+\t\tBUG(\"buffer too small\");\n+\treturn\thexval(lenbuf_hex[0]) << 12 |\n+\t\thexval(lenbuf_hex[1]) <<  8 |\n+\t\thexval(lenbuf_hex[2]) <<  4 |\n+\t\thexval(lenbuf_hex[3]);\n }\n\n static char *find_packfile_uri_path(const char *buffer)\n@@ -419,7 +423,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\treturn PACKET_READ_EOF;\n \t}\n\n-\tlen = packet_length(linelen);\n+\tlen = packet_length(linelen, sizeof(linelen));\n\n \tif (len < 0) {\n \t\tif (options & PACKET_READ_GENTLE_ON_READ_ERROR)\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 7c23a4bfaf..954eec8719 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -94,7 +94,7 @@ int packet_read(int fd, char *buffer, unsigned size, int options);\n  * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the\n  * numeric value of the length header.\n  */\n-int packet_length(const char lenbuf_hex[4]);\n+int packet_length(const char lenbuf_hex[4], size_t size);\n\n /*\n  * Read a packetized line into a buffer like the 'packet_read()' function but\ndiff --git a/remote-curl.c b/remote-curl.c\nindex acf7b2bb40..143318658e 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -763,7 +763,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n \t\t\tsize -= digits_remaining;\n\n \t\t\tif (state->len_filled == 4) {\n-\t\t\t\tstate->remaining = packet_length(state->len_buf);\n+\t\t\t\tstate->remaining = packet_length(state->len_buf,\n+\t\t\t\t\t\t\t\t sizeof(state->len_buf));\n \t\t\t\tif (state->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n \t\t\t\t} else if (state->remaining == 2) {\n--\n2.41.0\n"}]}