Re: [PATCH net] net/x25: prevent a couple of overflows
From: Martin Schiller <ms@dev.tdt.de>
Date: 2020-12-01 06:52:53
On 2020-11-30 11:04, Dan Carpenter wrote:
From: "kiyin(尹亮)" <redacted>
The .x25_addr[] address comes from the user and is not necessarily
NUL terminated. This leads to a couple problems. The first problem is
that the strlen() in x25_bind() can read beyond the end of the buffer.
The second problem is more subtle and could result in memory
corruption.
The call tree is:
x25_connect()
--> x25_write_internal()
--> x25_addr_aton()
The .x25_addr[] buffers are copied to the "addresses" buffer from
x25_write_internal() so it will lead to stack corruption.
The x25 protocol only allows 15 character addresses so putting a NUL
terminator as the 16th character is safe and obviously preferable to
reading out of bounds.
OK, I see the potential danger. I'm just wondering what is the better
approach here to counteract it:
1. check if the string is terminated or exceeds the maximum allowed
length and report an error if necessary.
2. always terminate the string at byte 15 as you suggested.
quoted hunk ↗ jump to hunk
Signed-off-by: "kiyin(尹亮)" <redacted> Signed-off-by: Dan Carpenter <redacted> --- net/x25/af_x25.c | 3 +++ 1 file changed, 3 insertions(+)diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index 0bbb283f23c9..3180f15942fe 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c@@ -686,6 +686,8 @@ static int x25_bind(struct socket *sock, structsockaddr *uaddr, int addr_len) goto out; } + addr->sx25_addr.x25_addr[X25_ADDR_LEN - 1] = '\0'; + /* check for the null_x25_address */ if (strcmp(addr->sx25_addr.x25_addr, null_x25_address.x25_addr)) {@@ -779,6 +781,7 @@ static int x25_connect(struct socket *sock, structsockaddr *uaddr, goto out; rc = -ENETUNREACH; + addr->sx25_addr.x25_addr[X25_ADDR_LEN - 1] = '\0'; rt = x25_get_route(&addr->sx25_addr); if (!rt) goto out;