~alpine/aports

1

[alpine-aports] [PATCH] main/musl: Fix out-of-bound read in sscanf

Marian Buschsieweke <marian.buschsieweke@ovgu.de>
Details
Message ID
<20190325092202.14077-1-marian.buschsieweke@ovgu.de>
Sender timestamp
1553505722
DKIM signature
missing
Download raw message
5 years ago
Patch: +61 -1 
Added patch from commit 8f12c4e110acb3bbbdc8abfb3a552c3ced718039, which
fixes an out-of-bound read in sscanf.
---
 main/musl/APKBUILD              |  4 ++-
 main/musl/sscanf_segfault.patch | 58 +++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 main/musl/sscanf_segfault.patch

diff --git a/main/musl/APKBUILD b/main/musl/APKBUILD
index 935520e46c..69aa8bbb0e 100644
--- a/main/musl/APKBUILD
+++ b/main/musl/APKBUILD
@@ -2,7 +2,7 @@
# Maintainer: Timo Teräs <timo.teras@iki.fi>
pkgname=musl
pkgver=1.1.21
pkgrel=1
pkgrel=2
pkgdesc="the musl c library (libc) implementation"
url="http://www.musl-libc.org/"
arch="all"
@@ -16,6 +16,7 @@ esac
source="http://www.musl-libc.org/releases/musl-$pkgver.tar.gz
	handle-aux-at_base.patch
	s390x-fadv.patch
	sscanf_segfault.patch

	ldconfig
	__stack_chk_fail_local.c
@@ -146,6 +147,7 @@ compat() {
sha512sums="fa6c4cc012626c5e517e0e10926fc845e3aa5f863ffaceeb38ac5b9ce0af631a37f6b94f470997db09aa0d5e03f4f28a2db83484b0f98481bea2239c1989d363  musl-1.1.21.tar.gz
6a7ff16d95b5d1be77e0a0fbb245491817db192176496a57b22ab037637d97a185ea0b0d19da687da66c2a2f5578e4343d230f399d49fe377d8f008410974238  handle-aux-at_base.patch
e9c9135f6dc3260e62ae6e9c45f3c43574af6ff2c2bfe411eb83f7e80d13bb8c86425cb41fc961e27f7bc15f679db1fbfb267e401bbe81d6cd5b872eb9b1f471  s390x-fadv.patch
8a5704b27f40d5b8700ba5355538cf16a5d6360e400231648fe1070d5fade9064d412432f62969e44179193283938731e79704575e085b2c7f10020ab977009b  sscanf_segfault.patch
8d3a2d5315fc56fee7da9abb8b89bb38c6046c33d154c10d168fb35bfde6b0cf9f13042a3bceee34daf091bc409d699223735dcf19f382eeee1f6be34154f26f  ldconfig
062bb49fa54839010acd4af113e20f7263dde1c8a2ca359b5fb2661ef9ed9d84a0f7c3bc10c25dcfa10bb3c5a4874588dff636ac43d5dbb3d748d75400756d0b  __stack_chk_fail_local.c
0d80f37b34a35e3d14b012257c50862dfeb9d2c81139ea2dfa101d981d093b009b9fa450ba27a708ac59377a48626971dfc58e20a3799084a65777a0c32cbc7d  getconf.c
diff --git a/main/musl/sscanf_segfault.patch b/main/musl/sscanf_segfault.patch
new file mode 100644
index 0000000000..629c70e1f6
--- /dev/null
+++ b/main/musl/sscanf_segfault.patch
@@ -0,0 +1,58 @@
From 8f12c4e110acb3bbbdc8abfb3a552c3ced718039 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Thu, 14 Mar 2019 20:52:18 -0400
Subject: fix crash/out-of-bound read in sscanf

commit d6c855caa88ddb1ab6e24e23a14b1e7baf4ba9c7 caused this
"regression", though the behavior was undefined before, overlooking
that f->shend=0 was being used as a sentinel for "EOF" status (actual
EOF or hitting the scanf field width) of the stream helper (shgetc)
functions.

obviously the shgetc macro could be adjusted to check for a null
pointer in addition to the != comparison, but it's the hot path, and
adding extra code/branches to it begins to defeat the purpose.

so instead of setting shend to a null pointer to block further reads,
which no longer works, set it to the current position (rpos). this
makes the shgetc macro work with no change, but it breaks shunget,
which can no longer look at the value of shend to determine whether to
back up. Szabolcs Nagy suggested a solution which I'm using here:
setting shlim to a negative value is inexpensive to test at shunget
time, and automatically re-trips the cnt>=shlim stop condition in
__shgetc no matter what the original limit was.
---
 src/internal/shgetc.c | 3 ++-
 src/internal/shgetc.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/internal/shgetc.c b/src/internal/shgetc.c
index ebd5fae7..a4a9c633 100644
--- a/src/internal/shgetc.c
+++ b/src/internal/shgetc.c
@@ -22,7 +22,8 @@ int __shgetc(FILE *f)
 	off_t cnt = shcnt(f);
 	if (f->shlim && cnt >= f->shlim || (c=__uflow(f)) < 0) {
 		f->shcnt = f->buf - f->rpos + cnt;
-		f->shend = 0;
+		f->shend = f->rpos;
+		f->shlim = -1;
 		return EOF;
 	}
 	cnt++;
diff --git a/src/internal/shgetc.h b/src/internal/shgetc.h
index 1c30f75f..9435381a 100644
--- a/src/internal/shgetc.h
+++ b/src/internal/shgetc.h
@@ -26,7 +26,7 @@ hidden int __shgetc(FILE *);
 #define shcnt(f) ((f)->shcnt + ((f)->rpos - (f)->buf))
 #define shlim(f, lim) __shlim((f), (lim))
 #define shgetc(f) (((f)->rpos != (f)->shend) ? *(f)->rpos++ : __shgetc(f))
-#define shunget(f) ((f)->shend ? (void)(f)->rpos-- : (void)0)
+#define shunget(f) ((f)->shlim>=0 ? (void)(f)->rpos-- : (void)0)
 
 #define sh_fromstring(f, s) \
 	((f)->buf = (f)->rpos = (void *)(s), (f)->rend = (void*)-1)
-- 
cgit v1.2.1

-- 
2.21.0



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20190325131758.61638409@ncopa-macbook.copa.dup.pw>
In-Reply-To
<20190325092202.14077-1-marian.buschsieweke@ovgu.de> (view parent)
Sender timestamp
1553519878
DKIM signature
missing
Download raw message
5 years ago 
On Mon, 25 Mar 2019 10:22:02 +0100
Marian Buschsieweke <marian.buschsieweke@ovgu.de> wrote:

> Added patch from commit 8f12c4e110acb3bbbdc8abfb3a552c3ced718039, which
> fixes an out-of-bound read in sscanf.
> ---
>  main/musl/APKBUILD              |  4 ++-
>  main/musl/sscanf_segfault.patch | 58 +++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 1 deletion(-)
>  create mode 100644 main/musl/sscanf_segfault.patch

This was applied to alpine edge, but apparently the patch does not
apply cleanly to musl 1.1.20, so I haven't backported it to alpine 3.9
yet.

Are there any bug report or test case related to this patch? Do you
know if musl 1.1.20 is affected as well?

-nc

> diff --git a/main/musl/APKBUILD b/main/musl/APKBUILD
> index 935520e46c..69aa8bbb0e 100644
> --- a/main/musl/APKBUILD
> +++ b/main/musl/APKBUILD
> @@ -2,7 +2,7 @@
>  # Maintainer: Timo Teräs <timo.teras@iki.fi>
>  pkgname=musl
>  pkgver=1.1.21
> -pkgrel=1
> +pkgrel=2
>  pkgdesc="the musl c library (libc) implementation"
>  url="http://www.musl-libc.org/"
>  arch="all"
> @@ -16,6 +16,7 @@ esac
>  source="http://www.musl-libc.org/releases/musl-$pkgver.tar.gz
>  	handle-aux-at_base.patch
>  	s390x-fadv.patch
> +	sscanf_segfault.patch
>  
>  	ldconfig
>  	__stack_chk_fail_local.c
> @@ -146,6 +147,7 @@ compat() {
>  sha512sums="fa6c4cc012626c5e517e0e10926fc845e3aa5f863ffaceeb38ac5b9ce0af631a37f6b94f470997db09aa0d5e03f4f28a2db83484b0f98481bea2239c1989d363  musl-1.1.21.tar.gz
>  6a7ff16d95b5d1be77e0a0fbb245491817db192176496a57b22ab037637d97a185ea0b0d19da687da66c2a2f5578e4343d230f399d49fe377d8f008410974238  handle-aux-at_base.patch
>  e9c9135f6dc3260e62ae6e9c45f3c43574af6ff2c2bfe411eb83f7e80d13bb8c86425cb41fc961e27f7bc15f679db1fbfb267e401bbe81d6cd5b872eb9b1f471  s390x-fadv.patch
> +8a5704b27f40d5b8700ba5355538cf16a5d6360e400231648fe1070d5fade9064d412432f62969e44179193283938731e79704575e085b2c7f10020ab977009b  sscanf_segfault.patch
>  8d3a2d5315fc56fee7da9abb8b89bb38c6046c33d154c10d168fb35bfde6b0cf9f13042a3bceee34daf091bc409d699223735dcf19f382eeee1f6be34154f26f  ldconfig
>  062bb49fa54839010acd4af113e20f7263dde1c8a2ca359b5fb2661ef9ed9d84a0f7c3bc10c25dcfa10bb3c5a4874588dff636ac43d5dbb3d748d75400756d0b  __stack_chk_fail_local.c
>  0d80f37b34a35e3d14b012257c50862dfeb9d2c81139ea2dfa101d981d093b009b9fa450ba27a708ac59377a48626971dfc58e20a3799084a65777a0c32cbc7d  getconf.c
> diff --git a/main/musl/sscanf_segfault.patch b/main/musl/sscanf_segfault.patch
> new file mode 100644
> index 0000000000..629c70e1f6
> --- /dev/null
> +++ b/main/musl/sscanf_segfault.patch
> @@ -0,0 +1,58 @@
> +From 8f12c4e110acb3bbbdc8abfb3a552c3ced718039 Mon Sep 17 00:00:00 2001
> +From: Rich Felker <dalias@aerifal.cx>
> +Date: Thu, 14 Mar 2019 20:52:18 -0400
> +Subject: fix crash/out-of-bound read in sscanf
> +
> +commit d6c855caa88ddb1ab6e24e23a14b1e7baf4ba9c7 caused this
> +"regression", though the behavior was undefined before, overlooking
> +that f->shend=0 was being used as a sentinel for "EOF" status (actual
> +EOF or hitting the scanf field width) of the stream helper (shgetc)
> +functions.
> +
> +obviously the shgetc macro could be adjusted to check for a null
> +pointer in addition to the != comparison, but it's the hot path, and
> +adding extra code/branches to it begins to defeat the purpose.
> +
> +so instead of setting shend to a null pointer to block further reads,
> +which no longer works, set it to the current position (rpos). this
> +makes the shgetc macro work with no change, but it breaks shunget,
> +which can no longer look at the value of shend to determine whether to
> +back up. Szabolcs Nagy suggested a solution which I'm using here:
> +setting shlim to a negative value is inexpensive to test at shunget
> +time, and automatically re-trips the cnt>=shlim stop condition in
> +__shgetc no matter what the original limit was.
> +---
> + src/internal/shgetc.c | 3 ++-
> + src/internal/shgetc.h | 2 +-
> + 2 files changed, 3 insertions(+), 2 deletions(-)
> +
> +diff --git a/src/internal/shgetc.c b/src/internal/shgetc.c
> +index ebd5fae7..a4a9c633 100644
> +--- a/src/internal/shgetc.c
> ++++ b/src/internal/shgetc.c
> +@@ -22,7 +22,8 @@ int __shgetc(FILE *f)
> + 	off_t cnt = shcnt(f);
> + 	if (f->shlim && cnt >= f->shlim || (c=__uflow(f)) < 0) {
> + 		f->shcnt = f->buf - f->rpos + cnt;
> +-		f->shend = 0;
> ++		f->shend = f->rpos;
> ++		f->shlim = -1;
> + 		return EOF;
> + 	}
> + 	cnt++;
> +diff --git a/src/internal/shgetc.h b/src/internal/shgetc.h
> +index 1c30f75f..9435381a 100644
> +--- a/src/internal/shgetc.h
> ++++ b/src/internal/shgetc.h
> +@@ -26,7 +26,7 @@ hidden int __shgetc(FILE *);
> + #define shcnt(f) ((f)->shcnt + ((f)->rpos - (f)->buf))
> + #define shlim(f, lim) __shlim((f), (lim))
> + #define shgetc(f) (((f)->rpos != (f)->shend) ? *(f)->rpos++ : __shgetc(f))
> +-#define shunget(f) ((f)->shend ? (void)(f)->rpos-- : (void)0)
> ++#define shunget(f) ((f)->shlim>=0 ? (void)(f)->rpos-- : (void)0)
> + 
> + #define sh_fromstring(f, s) \
> + 	((f)->buf = (f)->rpos = (void *)(s), (f)->rend = (void*)-1)
> +-- 
> +cgit v1.2.1
> +



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Reply to thread Export thread (mbox)