From: Tobias Stoeckmann <tobias@stoeckmann.org>
Subject: stravis: properly handle long strings
To: tech@openbsd.org
Date: Wed, 10 Jul 2024 18:52:46 +0200

Hi,

the stravis function can behave strangely if the visually encoded string
contains INT_MAX characters or more. This happens because the str*vis
functions use int for lengths.

To visually encode an input string, the input string length + 1 is
multiplied by 4 to have enough space for \xxx representation of a byte.
This is what stravis allocates first. After completion, it reallocs
the buffer to the actually required length, i.e. it truncates the memory.

But the length is stored in an int. Two scenarios could happen:

1. The returned length is -1
If len is -1, the "len + 1" expression is 0 and the buffer is actually
freed and its address stored in outp. This maybe sounds worse than it
is, because a -1 return value of stravis is considered to be an error
and I did not find a program in base which would use outp afterwards.

2. The string is longer than UINT_MAX
If it happens that the int value wraps around and becomes positive
again, the realloc actually removes the terminating NUL character, along
with many many more characters, of course. Calling printf on such a
pointer can easily trigger an out of bounds read.

To fix this, my proposed patch does the following:

1. Always set outp to a defined value (NULL by default)
2. Treat negative length as failure
3. If buf[len] is not the terminating NUL character, treat it as failure
4. Cast len to size_t to properly support exactly INT_MAX long strings

To see this in action, the ftp tool can be used. It requires lots of
heap memory though, so "ulimit -d unlimited" is your friend as root.
If a server replies with a redirect URL, the URL is passed to stravis if
the verbose command line argument has been passed.

See this proof of concept code:

1. The server (running on localhost 8080)

$ cat > poc.c << EOF
#include <limits.h>
#include <stdio.h>
#include <string.h>

int main(void) {
  char buf[4096 + 1];
  int i;

  memset(buf, 255, sizeof(buf));
  buf[sizeof(buf) - 1] = '\0';

  printf("status 301\nLocation: http://");
  for (i = 0; i < INT_MAX / 2 / 4096 + 1; i++)
    printf("%s", buf);
  printf("\n");
  return 0;
}
EOF
$ cc -o poc poc.c
$ ./poc | nc -l localhost 8080

2. The client (running as root)

# ulimit -d unlimited
# ftp -v http://localhost:8080/test
Trying 127.0.0.1...
Requesting http://localhost:8080/test
Redirected to http://\
ftp: : non-recoverable failure in name Resolution

The stravis bug can be seen in "Redirected to http://\" line which
is definitely not what the server replied.

With the patch applied, the error message states that not enough
memory is available. Although not entirely true, I think that it's
best to stay with current stravis error handling, which always sets
errno to ENOMEM in case of failure.

Okay?


Index: lib/libc/gen/vis.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/vis.c,v
diff -u -p -u -p -r1.26 vis.c
--- gen/vis.c	4 May 2022 18:57:50 -0000	1.26
+++ gen/vis.c	10 Jul 2024 16:05:51 -0000
@@ -245,11 +245,19 @@ stravis(char **outp, const char *src, in
 	int len, serrno;

 	buf = reallocarray(NULL, 4, strlen(src) + 1);
-	if (buf == NULL)
+	if (buf == NULL) {
+		*outp = NULL;
 		return -1;
+	}
 	len = strvis(buf, src, flag);
+	if (len < 0 || buf[len] != '\0') {
+		free(buf);
+		*outp = NULL;
+		errno = ENOMEM;
+		return -1;
+	}
 	serrno = errno;
-	*outp = realloc(buf, len + 1);
+	*outp = realloc(buf, (size_t)len + 1);
 	if (*outp == NULL) {
 		*outp = buf;
 		errno = serrno;