Message ID | 20150308204637.GA21863@aurel32.net |
---|---|
State | Superseded |
Headers |
Received: (qmail 61608 invoked by alias); 8 Mar 2015 20:46:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 61594 invoked by uid 89); 8 Mar 2015 20:46:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: hall.aurel32.net Date: Sun, 8 Mar 2015 21:46:37 +0100 From: Aurelien Jarno <aurelien@aurel32.net> To: libc-alpha@sourceware.org Subject: [PATCH][BUG 18093] Fix ldconfig segmentation fault with corrupted cache Message-ID: <20150308204637.GA21863@aurel32.net> Mail-Followup-To: libc-alpha@sourceware.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) |
Commit Message
Aurelien Jarno
March 8, 2015, 8:46 p.m. UTC
ldconfig is using an aux-cache to speed up the ld.so.cache update. It is read by mmaping the file to a structure which contains data offsets used as pointers. As they are not checked, it is not hard to get ldconfig to segfault with a corrupted file. This happens for instance if the file is truncated, which is common following a filesystem check following a system crash. This can be reproduced for example by truncating the file to roughly half of it's size. There is already in some code in elf/cache.c (load_aux_cache) to check for a corrupted aux cache, but it happens to be broken and not enough. The test (aux_cache->nlibs >= aux_cache_size) compares the number of libs entry with the cache size. It's a non sense, as it basically assumes that each library entry is a 1 byte... Instead the patch below computes the theoretical cache size using the headers and compares it to the real size. Another corruption can happen if the pointers to the string table is corrupted though in that case it means that the file has been more than just truncated. The patch below ignores entries pointing outside of the string table, they will be added with the current ldconfig run. (This patch is based on an initial patch from Lennart Sorensen). 2015-03-08 Aurelien Jarno <aurelien@aurel32.net> [BZ #18093] * elf/cache.c (load_aux_cache): Regenerate the cache if it has the wrong size. Ignore entries pointing outside of the mmaped memory.
Comments
On 03/08/2015 04:46 PM, Aurelien Jarno wrote: > ldconfig is using an aux-cache to speed up the ld.so.cache update. It > is read by mmaping the file to a structure which contains data offsets > used as pointers. As they are not checked, it is not hard to get > ldconfig to segfault with a corrupted file. This happens for instance if > the file is truncated, which is common following a filesystem check > following a system crash. That makes sense. > This can be reproduced for example by truncating the file to roughly > half of it's size. > > There is already in some code in elf/cache.c (load_aux_cache) to check > for a corrupted aux cache, but it happens to be broken and not enough. > The test (aux_cache->nlibs >= aux_cache_size) compares the number of > libs entry with the cache size. It's a non sense, as it basically > assumes that each library entry is a 1 byte... Instead the patch below > computes the theoretical cache size using the headers and compares it > to the real size. This is OK. > Another corruption can happen if the pointers to the string table is > corrupted though in that case it means that the file has been more than > just truncated. The patch below ignores entries pointing outside of the > string table, they will be added with the current ldconfig run. This is not OK, since it requires having incorrectly modified the file which we assume cannot happen and slows down the cache access. > (This patch is based on an initial patch from Lennart Sorensen). > > > 2015-03-08 Aurelien Jarno <aurelien@aurel32.net> > > [BZ #18093] > * elf/cache.c (load_aux_cache): Regenerate the cache if it has the > wrong size. Ignore entries pointing outside of the mmaped memory. > > diff --git a/elf/cache.c b/elf/cache.c > index 1732268..9131e08 100644 > --- a/elf/cache.c > +++ b/elf/cache.c > @@ -698,7 +698,9 @@ load_aux_cache (const char *aux_cache_name) > if (aux_cache == MAP_FAILED > || aux_cache_size < sizeof (struct aux_cache_file) > || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1) > - || aux_cache->nlibs >= aux_cache_size) > + || aux_cache_size != (sizeof(struct aux_cache_file) + > + aux_cache->nlibs * sizeof(struct aux_cache_file_entry) + > + aux_cache->len_strings)) I'm not happy to have "!=" in the event the file is larger, but has zero padding, thus I would like to see `aux_cache_size >=`. > { > close (fd); > init_aux_cache (); > @@ -711,12 +713,13 @@ load_aux_cache (const char *aux_cache_name) > const char *aux_cache_data > = (const char *) &aux_cache->libs[aux_cache->nlibs]; > for (unsigned int i = 0; i < aux_cache->nlibs; ++i) > - insert_to_aux_cache (&aux_cache->libs[i].id, > - aux_cache->libs[i].flags, > - aux_cache->libs[i].osversion, > - aux_cache->libs[i].soname == 0 > - ? NULL : aux_cache_data + aux_cache->libs[i].soname, > - 0); > + if (aux_cache->libs[i].soname < aux_cache->len_strings) > + insert_to_aux_cache (&aux_cache->libs[i].id, > + aux_cache->libs[i].flags, > + aux_cache->libs[i].osversion, > + aux_cache->libs[i].soname == 0 > + ? NULL : aux_cache_data + aux_cache->libs[i].soname, > + 0); Drop this, we don't need it. You've not shown how the file could get corrupted, and therefore this check is not needed and slows down cache loading. > > munmap (aux_cache, aux_cache_size); > close (fd); > Please repost v2 for review. Cheers, Carlos.
On Sun, Mar 8, 2015 at 8:46 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: > ldconfig is using an aux-cache to speed up the ld.so.cache update. It > is read by mmaping the file to a structure which contains data offsets > used as pointers. As they are not checked, it is not hard to get > ldconfig to segfault with a corrupted file. This happens for instance if > the file is truncated, which is common following a filesystem check > following a system crash. A similar issue can also occur with qemu user running e.g. big endian binaries on a little endian host. I don't know if there is a good fix for that (or even if it is worth coming up with one). > This can be reproduced for example by truncating the file to roughly > half of it's size. > > There is already in some code in elf/cache.c (load_aux_cache) to check > for a corrupted aux cache, but it happens to be broken and not enough. > The test (aux_cache->nlibs >= aux_cache_size) compares the number of > libs entry with the cache size. It's a non sense, as it basically > assumes that each library entry is a 1 byte... Instead the patch below > computes the theoretical cache size using the headers and compares it > to the real size. > > Another corruption can happen if the pointers to the string table is > corrupted though in that case it means that the file has been more than > just truncated. The patch below ignores entries pointing outside of the > string table, they will be added with the current ldconfig run. > > (This patch is based on an initial patch from Lennart Sorensen). > > > 2015-03-08 Aurelien Jarno <aurelien@aurel32.net> > > [BZ #18093] > * elf/cache.c (load_aux_cache): Regenerate the cache if it has the > wrong size. Ignore entries pointing outside of the mmaped memory. > > diff --git a/elf/cache.c b/elf/cache.c > index 1732268..9131e08 100644 > --- a/elf/cache.c > +++ b/elf/cache.c > @@ -698,7 +698,9 @@ load_aux_cache (const char *aux_cache_name) > if (aux_cache == MAP_FAILED > || aux_cache_size < sizeof (struct aux_cache_file) > || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1) > - || aux_cache->nlibs >= aux_cache_size) > + || aux_cache_size != (sizeof(struct aux_cache_file) + > + aux_cache->nlibs * sizeof(struct aux_cache_file_entry) + > + aux_cache->len_strings)) > { > close (fd); > init_aux_cache (); > @@ -711,12 +713,13 @@ load_aux_cache (const char *aux_cache_name) > const char *aux_cache_data > = (const char *) &aux_cache->libs[aux_cache->nlibs]; > for (unsigned int i = 0; i < aux_cache->nlibs; ++i) > - insert_to_aux_cache (&aux_cache->libs[i].id, > - aux_cache->libs[i].flags, > - aux_cache->libs[i].osversion, > - aux_cache->libs[i].soname == 0 > - ? NULL : aux_cache_data + aux_cache->libs[i].soname, > - 0); > + if (aux_cache->libs[i].soname < aux_cache->len_strings) > + insert_to_aux_cache (&aux_cache->libs[i].id, > + aux_cache->libs[i].flags, > + aux_cache->libs[i].osversion, > + aux_cache->libs[i].soname == 0 > + ? NULL : aux_cache_data + aux_cache->libs[i].soname, > + 0); > > munmap (aux_cache, aux_cache_size); > close (fd); > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net
On 2015-03-09 09:06, Carlos O'Donell wrote: > On 03/08/2015 04:46 PM, Aurelien Jarno wrote: > > ldconfig is using an aux-cache to speed up the ld.so.cache update. It > > is read by mmaping the file to a structure which contains data offsets > > used as pointers. As they are not checked, it is not hard to get > > ldconfig to segfault with a corrupted file. This happens for instance if > > the file is truncated, which is common following a filesystem check > > following a system crash. > > That makes sense. > > > This can be reproduced for example by truncating the file to roughly > > half of it's size. > > > > There is already in some code in elf/cache.c (load_aux_cache) to check > > for a corrupted aux cache, but it happens to be broken and not enough. > > The test (aux_cache->nlibs >= aux_cache_size) compares the number of > > libs entry with the cache size. It's a non sense, as it basically > > assumes that each library entry is a 1 byte... Instead the patch below > > computes the theoretical cache size using the headers and compares it > > to the real size. > > This is OK. > > > Another corruption can happen if the pointers to the string table is > > corrupted though in that case it means that the file has been more than > > just truncated. The patch below ignores entries pointing outside of the > > string table, they will be added with the current ldconfig run. > > This is not OK, since it requires having incorrectly modified the file > which we assume cannot happen and slows down the cache access. While I don't have a scenario leading to this, it seems it could happen in practice. I am not really sure we want to see segmentation faults happening in such cases. As for the speed, I don't think it's a good argument here. We talk about the ldconfig aux cache here not the ld.so.cache where the speed is more critical. The patch only adds one integer comparison for each loop, while the loop contains calls to malloc or memcpy which take a magnitude more time to execute. I don't think the effect is measurable. > > (This patch is based on an initial patch from Lennart Sorensen). > > > > > > 2015-03-08 Aurelien Jarno <aurelien@aurel32.net> > > > > [BZ #18093] > > * elf/cache.c (load_aux_cache): Regenerate the cache if it has the > > wrong size. Ignore entries pointing outside of the mmaped memory. > > > > diff --git a/elf/cache.c b/elf/cache.c > > index 1732268..9131e08 100644 > > --- a/elf/cache.c > > +++ b/elf/cache.c > > @@ -698,7 +698,9 @@ load_aux_cache (const char *aux_cache_name) > > if (aux_cache == MAP_FAILED > > || aux_cache_size < sizeof (struct aux_cache_file) > > || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1) > > - || aux_cache->nlibs >= aux_cache_size) > > + || aux_cache_size != (sizeof(struct aux_cache_file) + > > + aux_cache->nlibs * sizeof(struct aux_cache_file_entry) + > > + aux_cache->len_strings)) > > I'm not happy to have "!=" in the event the file is larger, but has > zero padding, thus I would like to see `aux_cache_size >=`. While would the file get suddenly zero padded? If anything changes the aux-cache without touching the headers accordingly, it's probably safer to just regenerate it (even it it takes time) than risking introducing corrupted entries in the ld.so.cache. Cheers, Aurelien
On 2015-03-09 13:37, Will Newton wrote: > On Sun, Mar 8, 2015 at 8:46 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: > > ldconfig is using an aux-cache to speed up the ld.so.cache update. It > > is read by mmaping the file to a structure which contains data offsets > > used as pointers. As they are not checked, it is not hard to get > > ldconfig to segfault with a corrupted file. This happens for instance if > > the file is truncated, which is common following a filesystem check > > following a system crash. > > A similar issue can also occur with qemu user running e.g. big endian > binaries on a little endian host. I don't know if there is a good fix > for that (or even if it is worth coming up with one). This patch would likely workaround the issue for the aux-cache. The sizes read from the file are inconsistent in such case, and thus the file would be regenerated. Not very nice from the performance point of view, but that should work. That said I guess that there is the same problem at the ld.so.cache level, and this patch will not help for that. Cheers, Aurelien
On 03/09/2015 04:11 PM, Aurelien Jarno wrote: > On 2015-03-09 09:06, Carlos O'Donell wrote: >> On 03/08/2015 04:46 PM, Aurelien Jarno wrote: >>> ldconfig is using an aux-cache to speed up the ld.so.cache update. It >>> is read by mmaping the file to a structure which contains data offsets >>> used as pointers. As they are not checked, it is not hard to get >>> ldconfig to segfault with a corrupted file. This happens for instance if >>> the file is truncated, which is common following a filesystem check >>> following a system crash. >> >> That makes sense. >> >>> This can be reproduced for example by truncating the file to roughly >>> half of it's size. >>> >>> There is already in some code in elf/cache.c (load_aux_cache) to check >>> for a corrupted aux cache, but it happens to be broken and not enough. >>> The test (aux_cache->nlibs >= aux_cache_size) compares the number of >>> libs entry with the cache size. It's a non sense, as it basically >>> assumes that each library entry is a 1 byte... Instead the patch below >>> computes the theoretical cache size using the headers and compares it >>> to the real size. >> >> This is OK. >> >>> Another corruption can happen if the pointers to the string table is >>> corrupted though in that case it means that the file has been more than >>> just truncated. The patch below ignores entries pointing outside of the >>> string table, they will be added with the current ldconfig run. >> >> This is not OK, since it requires having incorrectly modified the file >> which we assume cannot happen and slows down the cache access. > > While I don't have a scenario leading to this, it seems it could happen > in practice. I am not really sure we want to see segmentation faults > happening in such cases. How could it happen? Filesystem corruption? Then you've got a lot of potentially unknown corruption to contend with, why should glibc try to exchange performance for robustness if the filesystem failed? > As for the speed, I don't think it's a good argument here. We talk about > the ldconfig aux cache here not the ld.so.cache where the speed is more > critical. The patch only adds one integer comparison for each loop, > while the loop contains calls to malloc or memcpy which take a magnitude > more time to execute. I don't think the effect is measurable. You are arguing to increase the complexity of code and reduce the speed (even if it's small) for a potentially low-probability event that already has disastrous consequences for the entire system. If you could come up with even one non-filesystem-corruption event that caused this problem I might consider it a good change. >>> (This patch is based on an initial patch from Lennart Sorensen). >>> >>> >>> 2015-03-08 Aurelien Jarno <aurelien@aurel32.net> >>> >>> [BZ #18093] >>> * elf/cache.c (load_aux_cache): Regenerate the cache if it has the >>> wrong size. Ignore entries pointing outside of the mmaped memory. >>> >>> diff --git a/elf/cache.c b/elf/cache.c >>> index 1732268..9131e08 100644 >>> --- a/elf/cache.c >>> +++ b/elf/cache.c >>> @@ -698,7 +698,9 @@ load_aux_cache (const char *aux_cache_name) >>> if (aux_cache == MAP_FAILED >>> || aux_cache_size < sizeof (struct aux_cache_file) >>> || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1) >>> - || aux_cache->nlibs >= aux_cache_size) >>> + || aux_cache_size != (sizeof(struct aux_cache_file) + >>> + aux_cache->nlibs * sizeof(struct aux_cache_file_entry) + >>> + aux_cache->len_strings)) >> >> I'm not happy to have "!=" in the event the file is larger, but has >> zero padding, thus I would like to see `aux_cache_size >=`. > > While would the file get suddenly zero padded? If anything changes the > aux-cache without touching the headers accordingly, it's probably safer > to just regenerate it (even it it takes time) than risking introducing > corrupted entries in the ld.so.cache. I withdraw my complaint. The two other good reasons I could come up with are that it makes hacking the file easier, and it makes it future proof to allow us to tack on a new header at the end. However, neither of those are good enough given that the if the additional data can be written to, it becomes a security problem, whereas if the mapping was exactly the size required it would make things harder. Cheers, Carlos.
diff --git a/elf/cache.c b/elf/cache.c index 1732268..9131e08 100644 --- a/elf/cache.c +++ b/elf/cache.c @@ -698,7 +698,9 @@ load_aux_cache (const char *aux_cache_name) if (aux_cache == MAP_FAILED || aux_cache_size < sizeof (struct aux_cache_file) || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1) - || aux_cache->nlibs >= aux_cache_size) + || aux_cache_size != (sizeof(struct aux_cache_file) + + aux_cache->nlibs * sizeof(struct aux_cache_file_entry) + + aux_cache->len_strings)) { close (fd); init_aux_cache (); @@ -711,12 +713,13 @@ load_aux_cache (const char *aux_cache_name) const char *aux_cache_data = (const char *) &aux_cache->libs[aux_cache->nlibs]; for (unsigned int i = 0; i < aux_cache->nlibs; ++i) - insert_to_aux_cache (&aux_cache->libs[i].id, - aux_cache->libs[i].flags, - aux_cache->libs[i].osversion, - aux_cache->libs[i].soname == 0 - ? NULL : aux_cache_data + aux_cache->libs[i].soname, - 0); + if (aux_cache->libs[i].soname < aux_cache->len_strings) + insert_to_aux_cache (&aux_cache->libs[i].id, + aux_cache->libs[i].flags, + aux_cache->libs[i].osversion, + aux_cache->libs[i].soname == 0 + ? NULL : aux_cache_data + aux_cache->libs[i].soname, + 0); munmap (aux_cache, aux_cache_size); close (fd);