The following patch fixes a 3 bytes info leak via getsockname() on
AF_NETROM socket.
Details:
typedef struct {
char ax25_call[7]; /* 6 call + SSID (shifted ascii!) */
} ax25_address;
struct sockaddr_ax25 {
__kernel_sa_family_t sax25_family;
ax25_address sax25_call;
int sax25_ndigis;
/* Digipeater ax25_address sets follow */
};
After compilation, gcc will add 3 padding bytes after sax25_call to
align sax25_ndigis and nr_getname does not clear these padding bytes
before returning to userland, poc attached.
Signed-off-by: Clement Lecigne <redacted>
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -838,6 +838,8 @@ static int nr_getname(struct socket *sock, struct
sockaddr *uaddr,
struct sock *sk = sock->sk;
struct nr_sock *nr = nr_sk(sk);
+ memset(sax, 0, sizeof(struct sockaddr_ax25));
+
lock_sock(sk);
if (peer != 0) {
if (sk->sk_state != TCP_ESTABLISHED) {
On Sun, 2012-10-14 at 20:23 +0200, Clément Lecigne wrote:
quoted hunk
The following patch fixes a 3 bytes info leak via getsockname() on
AF_NETROM socket.
Details:
typedef struct {
char ax25_call[7]; /* 6 call + SSID (shifted ascii!) */
} ax25_address;
struct sockaddr_ax25 {
__kernel_sa_family_t sax25_family;
ax25_address sax25_call;
int sax25_ndigis;
/* Digipeater ax25_address sets follow */
};
After compilation, gcc will add 3 padding bytes after sax25_call to
align sax25_ndigis and nr_getname does not clear these padding bytes
before returning to userland, poc attached.
Signed-off-by: Clement Lecigne <redacted>
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -838,6 +838,8 @@ static int nr_getname(struct socket *sock, struct
sockaddr *uaddr,
struct sock *sk = sock->sk;
struct nr_sock *nr = nr_sk(sk);
+ memset(sax, 0, sizeof(struct sockaddr_ax25));
This is not indented correctly.
lock_sock(sk);
if (peer != 0) {
if (sk->sk_state != TCP_ESTABLISHED) {
On some architectures, structures have a minimum alignment of 4, so
there are 2 bytes of padding before sax25_call and 1 byte within it.
nr_getname() copies complete ax25_address structures, but nr_listen()
does not initialise the 1 byte of padding in user_addr:
memset(&nr_sk(sk)->user_addr, 0, AX25_ADDR_LEN);
So I think you should address that too. It appears that other
ax25_address structures are originally copied from userland and
therefore don't leak information, but I'm not sure.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Ben, thanks for your reply, I fixed the indentation and I addressed
the possible leak in nr_listen() too. Here is the patch.
Signed-off-by: Clement Lecigne <redacted>
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -408,7 +408,7 @@ static int nr_listen(struct socket *sock, int backlog)
lock_sock(sk);
if (sk->sk_state != TCP_LISTEN) {
- memset(&nr_sk(sk)->user_addr, 0, AX25_ADDR_LEN);
+ memset(&nr_sk(sk)->user_addr, 0, sizeof(nr_sk(sk)->user_addr));
sk->sk_max_ack_backlog = backlog;
sk->sk_state = TCP_LISTEN;
release_sock(sk);@@ -838,6 +838,8 @@ static int nr_getname(struct socket *sock, struct
sockaddr *uaddr,
struct sock *sk = sock->sk;
struct nr_sock *nr = nr_sk(sk);
+ memset(sax, 0, sizeof(struct sockaddr_ax25));
+
lock_sock(sk);
if (peer != 0) {
if (sk->sk_state != TCP_ESTABLISHED) {
2012/10/15 Ben Hutchings [off-list ref]:On Sun, 2012-10-14 at 20:23 +0200, Clément Lecigne wrote:
quoted
The following patch fixes a 3 bytes info leak via getsockname() on
AF_NETROM socket.
Details:
typedef struct {
char ax25_call[7]; /* 6 call + SSID (shifted ascii!) */
} ax25_address;
struct sockaddr_ax25 {
__kernel_sa_family_t sax25_family;
ax25_address sax25_call;
int sax25_ndigis;
/* Digipeater ax25_address sets follow */
};
After compilation, gcc will add 3 padding bytes after sax25_call to
align sax25_ndigis and nr_getname does not clear these padding bytes
before returning to userland, poc attached.
Signed-off-by: Clement Lecigne <redacted>
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -838,6 +838,8 @@ static int nr_getname(struct socket *sock, struct
sockaddr *uaddr,
struct sock *sk = sock->sk;
struct nr_sock *nr = nr_sk(sk);
+ memset(sax, 0, sizeof(struct sockaddr_ax25));
This is not indented correctly.
quoted
lock_sock(sk);
if (peer != 0) {
if (sk->sk_state != TCP_ESTABLISHED) {
On some architectures, structures have a minimum alignment of 4, so
there are 2 bytes of padding before sax25_call and 1 byte within it.
nr_getname() copies complete ax25_address structures, but nr_listen()
does not initialise the 1 byte of padding in user_addr:
memset(&nr_sk(sk)->user_addr, 0, AX25_ADDR_LEN);
So I think you should address that too. It appears that other
ax25_address structures are originally copied from userland and
therefore don't leak information, but I'm not sure.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.