| Message ID | 20231205235919.510051-4-adam.duskett@amarulasolutions.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | package/giflib security fixes | expand |
Adam, All, On 2023-12-05 16:59 -0700, Adam Duskett spake thusly: > Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> Applied to master, thanks. Regards, Yann E. MORIN. > --- > ...veral-defects-found-by-Coverity-scan.patch | 61 +++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100644 package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > diff --git a/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > new file mode 100644 > index 0000000000..1719769872 > --- /dev/null > +++ b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > @@ -0,0 +1,61 @@ > +From a1c48b91cd1cf1e9bf7077709b69f4bfd4c4abc7 Mon Sep 17 00:00:00 2001 > +From: Sandro Mani <manisandro@gmail.com> > +Date: Tue, 5 Dec 2023 16:38:48 -0700 > +Subject: [PATCH] Fix several defects found by Coverity scan > + > +From: giflib-5.2.1-17.fc39.src.rpm > +Upstream: Not submitted > + > +Signed-off-by: Sandro Mani <manisandro@gmail.com> > +Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> > +--- > + gif2rgb.c | 11 ++++++++++- > + 1 file changed, 10 insertions(+), 1 deletion(-) > + > +diff --git a/gif2rgb.c b/gif2rgb.c > +index d9a469f..02cea41 100644 > +--- a/gif2rgb.c > ++++ b/gif2rgb.c > +@@ -170,6 +170,8 @@ static void SaveGif(GifByteType *OutputBuffer, > + /* Open stdout for the output file: */ > + if ((GifFile = EGifOpenFileHandle(1, &Error)) == NULL) { > + PrintGifError(Error); > ++ free(OutputBuffer); > ++ GifFreeMapObject(OutputColorMap); > + exit(EXIT_FAILURE); > + } > + > +@@ -179,6 +181,8 @@ static void SaveGif(GifByteType *OutputBuffer, > + EGifPutImageDesc(GifFile, > + 0, 0, Width, Height, false, NULL) == GIF_ERROR) { > + PrintGifError(Error); > ++ free(OutputBuffer); > ++ GifFreeMapObject(OutputColorMap); > + exit(EXIT_FAILURE); > + } > + > +@@ -187,8 +191,11 @@ static void SaveGif(GifByteType *OutputBuffer, > + GifFile->Image.Width, GifFile->Image.Height); > + > + for (i = 0; i < Height; i++) { > +- if (EGifPutLine(GifFile, Ptr, Width) == GIF_ERROR) > ++ if (EGifPutLine(GifFile, Ptr, Width) == GIF_ERROR) { > ++ free(OutputBuffer); > ++ GifFreeMapObject(OutputColorMap); > + exit(EXIT_FAILURE); > ++ } > + GifQprintf("\b\b\b\b%-4d", Height - i - 1); > + > + Ptr += Width; > +@@ -196,6 +203,8 @@ static void SaveGif(GifByteType *OutputBuffer, > + > + if (EGifCloseFile(GifFile, &Error) == GIF_ERROR) { > + PrintGifError(Error); > ++ free(OutputBuffer); > ++ GifFreeMapObject(OutputColorMap); > + exit(EXIT_FAILURE); > + } > + } > +-- > +2.43.0 > + > -- > 2.43.0 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
>>>>> "Adam" == Adam Duskett <adam.duskett@amarulasolutions.com> writes: > Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> > --- > ...veral-defects-found-by-Coverity-scan.patch | 61 +++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100644 package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > diff --git a/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > new file mode 100644 > index 0000000000..1719769872 > --- /dev/null > +++ b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > @@ -0,0 +1,61 @@ > +From a1c48b91cd1cf1e9bf7077709b69f4bfd4c4abc7 Mon Sep 17 00:00:00 2001 > +From: Sandro Mani <manisandro@gmail.com> > +Date: Tue, 5 Dec 2023 16:38:48 -0700 > +Subject: [PATCH] Fix several defects found by Coverity scan > + > +From: giflib-5.2.1-17.fc39.src.rpm > +Upstream: Not submitted No upstream and no CVE? Where does this fix then come from?
Peter, All, On 2024-01-07 10:29 +0100, Peter Korsgaard spake thusly: > >>>>> "Adam" == Adam Duskett <adam.duskett@amarulasolutions.com> writes: > > Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> > > --- > > ...veral-defects-found-by-Coverity-scan.patch | 61 +++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > create mode 100644 package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > > diff --git a/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > new file mode 100644 > > index 0000000000..1719769872 > > --- /dev/null > > +++ b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > @@ -0,0 +1,61 @@ > > +From a1c48b91cd1cf1e9bf7077709b69f4bfd4c4abc7 Mon Sep 17 00:00:00 2001 > > +From: Sandro Mani <manisandro@gmail.com> > > +Date: Tue, 5 Dec 2023 16:38:48 -0700 > > +Subject: [PATCH] Fix several defects found by Coverity scan > > + > > +From: giflib-5.2.1-17.fc39.src.rpm > > +Upstream: Not submitted > > No upstream and no CVE? Where does this fix then come from? I was a bit sloppy when applying that one, indeed. As the commit log mention, it's taken from the Fedora 39 source package, and I believed it was enough reference. Looking at that source package, it matches the patch named giflib_coverity.patch and the Fedora dist-git for that patch date back to 2020-02-17: https://src.fedoraproject.org/rpms/giflib/c/df94d26a07ac8772b3380f4e5b4145daa7bf65e1?branch=rawhide As far as I could find, it has not been submitted upstream, and upstream looks like it has been pretty mothballed for a while now; last commit was on 2019-08-17: https://sourceforge.net/p/giflib/mailman/giflib-devel/ https://sourceforge.net/p/giflib/code/ci/master/tree/ I could not find any associated CVE: https://nvd.nist.gov/vuln/search/results?adv_search=true&isCpeNameSearch=true&query=cpe%3A2.3%3Aa%3Agiflib_project%3Agiflib%3A5.2.1%3A*%3A*%3A*%3A*%3A*%3A*%3A* Looking at the code, I doubt it is a security issue, in fact. It's probably just a memory leak, as the free() is replaced by this function: 79 void 80 GifFreeMapObject(ColorMapObject *Object) 81 { 82 if (Object != NULL) { 83 (void)free(Object->Colors); 84 (void)free(Object); 85 } 86 } So, Object->Colors leaked, but I don't think it was a "security" issue. Regards, Yann E. MORIN.
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > Peter, All, > On 2024-01-07 10:29 +0100, Peter Korsgaard spake thusly: >> >>>>> "Adam" == Adam Duskett <adam.duskett@amarulasolutions.com> writes: >> > Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> >> > --- >> > ...veral-defects-found-by-Coverity-scan.patch | 61 +++++++++++++++++++ >> > 1 file changed, 61 insertions(+) >> > create mode 100644 package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch >> >> > diff --git >> > a/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch >> > b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch >> > new file mode 100644 >> > index 0000000000..1719769872 >> > --- /dev/null >> > +++ b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch >> > @@ -0,0 +1,61 @@ >> > +From a1c48b91cd1cf1e9bf7077709b69f4bfd4c4abc7 Mon Sep 17 00:00:00 2001 >> > +From: Sandro Mani <manisandro@gmail.com> >> > +Date: Tue, 5 Dec 2023 16:38:48 -0700 >> > +Subject: [PATCH] Fix several defects found by Coverity scan >> > + >> > +From: giflib-5.2.1-17.fc39.src.rpm >> > +Upstream: Not submitted >> >> No upstream and no CVE? Where does this fix then come from? > I was a bit sloppy when applying that one, indeed. As the commit log > mention, it's taken from the Fedora 39 source package, and I believed it > was enough reference. > Looking at that source package, it matches the patch named giflib_coverity.patch > and the Fedora dist-git for that patch date back to 2020-02-17: > https://src.fedoraproject.org/rpms/giflib/c/df94d26a07ac8772b3380f4e5b4145daa7bf65e1?branch=rawhide > As far as I could find, it has not been submitted upstream, and upstream > looks like it has been pretty mothballed for a while now; last commit > was on 2019-08-17: > https://sourceforge.net/p/giflib/mailman/giflib-devel/ > https://sourceforge.net/p/giflib/code/ci/master/tree/ > I could not find any associated CVE: > https://nvd.nist.gov/vuln/search/results?adv_search=true&isCpeNameSearch=true&query=cpe%3A2.3%3Aa%3Agiflib_project%3Agiflib%3A5.2.1%3A*%3A*%3A*%3A*%3A*%3A*%3A* > Looking at the code, I doubt it is a security issue, in fact. It's > probably just a memory leak, as the free() is replaced by this function: > 79 void > 80 GifFreeMapObject(ColorMapObject *Object) > 81 { > 82 if (Object != NULL) { > 83 (void)free(Object->Colors); > 84 (void)free(Object); > 85 } > 86 } > So, Object->Colors leaked, but I don't think it was a "security" issue. Ok, thanks for the details. I'll add it anyway to the backports for consistency.
On Sun, 7 Jan 2024 13:10:30 +0100, "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > Peter, All, > > On 2024-01-07 10:29 +0100, Peter Korsgaard spake thusly: > > >>>>> "Adam" == Adam Duskett <adam.duskett@amarulasolutions.com> writes: > > > Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> > > > --- > > > ...veral-defects-found-by-Coverity-scan.patch | 61 +++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > create mode 100644 package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > > > > diff --git a/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > > new file mode 100644 > > > index 0000000000..1719769872 > > > --- /dev/null > > > +++ b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch > > > @@ -0,0 +1,61 @@ > > > +From a1c48b91cd1cf1e9bf7077709b69f4bfd4c4abc7 Mon Sep 17 00:00:00 2001 > > > +From: Sandro Mani <manisandro@gmail.com> > > > +Date: Tue, 5 Dec 2023 16:38:48 -0700 > > > +Subject: [PATCH] Fix several defects found by Coverity scan > > > + > > > +From: giflib-5.2.1-17.fc39.src.rpm > > > +Upstream: Not submitted > > > > No upstream and no CVE? Where does this fix then come from? > > I was a bit sloppy when applying that one, indeed. As the commit log > mention, it's taken from the Fedora 39 source package, and I believed it > was enough reference. > > Looking at that source package, it matches the patch named giflib_coverity.patch > and the Fedora dist-git for that patch date back to 2020-02-17: > https://src.fedoraproject.org/rpms/giflib/c/df94d26a07ac8772b3380f4e5b4145daa7bf65e1?branch=rawhide > > As far as I could find, it has not been submitted upstream, and upstream > looks like it has been pretty mothballed for a while now; last commit > was on 2019-08-17: > https://sourceforge.net/p/giflib/mailman/giflib-devel/ > https://sourceforge.net/p/giflib/code/ci/master/tree/ > > I could not find any associated CVE: > > https://nvd.nist.gov/vuln/search/results?adv_search=true&isCpeNameSearch=true&query=cpe%3A2.3%3Aa%3Agiflib_project%3Agiflib%3A5.2.1%3A*%3A*%3A*%3A*%3A*%3A*%3A* > > Looking at the code, I doubt it is a security issue, in fact. It's > probably just a memory leak, as the free() is replaced by this function: > > 79 void > 80 GifFreeMapObject(ColorMapObject *Object) > 81 { > 82 if (Object != NULL) { > 83 (void)free(Object->Colors); > 84 (void)free(Object); > 85 } > 86 } > > So, Object->Colors leaked, but I don't think it was a "security" issue. Matter of judgment if a (very theoretically) denial-of-service/out-of-memory is counted as 'security' issue ;-) Regards, Peter > > Regards, > Yann E. MORIN. >
Peter, All, On 2024-01-07 21:03 +0100, Peter Seiderer spake thusly: > On Sun, 7 Jan 2024 13:10:30 +0100, "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: [--SNIP--] > > Looking at the code, I doubt it is a security issue, in fact. It's > > probably just a memory leak, as the free() is replaced by this function: [--SNIP--] > > So, Object->Colors leaked, but I don't think it was a "security" issue. > Matter of judgment if a (very theoretically) denial-of-service/out-of-memory is counted > as 'security' issue ;-) Sure, but the affected code is in the gif2rgb program, not in the library, so the effects of the memory leak are limited to the time the program runs. gif2rgb only handles a single intput file, so it is short-lived, and the leaked memory is reclaimed once the program is reaped. Also note that the program does an exit() right after freeing that memory; it does not ensure that any other allocated memory is freed before exiting, so there might anyway be more memory that leaks. So yes, it's better do free memory with the semantically corresponding funtion (GifMapObject() -> GifFreeMapObject()), but it would have real difficulties getting classified as a security issue *in this specific case*. The only way this leaked memory could cause an issue, is if the program is not reaped. But then, if someone is able to attack the system by leaving zombies around, whether those zombies leaked memory or not is of no concern. So I think, but of course, security is so difficult that I may easily be wrong. Regards, Yann E. MORIN.
diff --git a/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch new file mode 100644 index 0000000000..1719769872 --- /dev/null +++ b/package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch @@ -0,0 +1,61 @@ +From a1c48b91cd1cf1e9bf7077709b69f4bfd4c4abc7 Mon Sep 17 00:00:00 2001 +From: Sandro Mani <manisandro@gmail.com> +Date: Tue, 5 Dec 2023 16:38:48 -0700 +Subject: [PATCH] Fix several defects found by Coverity scan + +From: giflib-5.2.1-17.fc39.src.rpm +Upstream: Not submitted + +Signed-off-by: Sandro Mani <manisandro@gmail.com> +Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> +--- + gif2rgb.c | 11 ++++++++++- + 1 file changed, 10 insertions(+), 1 deletion(-) + +diff --git a/gif2rgb.c b/gif2rgb.c +index d9a469f..02cea41 100644 +--- a/gif2rgb.c ++++ b/gif2rgb.c +@@ -170,6 +170,8 @@ static void SaveGif(GifByteType *OutputBuffer, + /* Open stdout for the output file: */ + if ((GifFile = EGifOpenFileHandle(1, &Error)) == NULL) { + PrintGifError(Error); ++ free(OutputBuffer); ++ GifFreeMapObject(OutputColorMap); + exit(EXIT_FAILURE); + } + +@@ -179,6 +181,8 @@ static void SaveGif(GifByteType *OutputBuffer, + EGifPutImageDesc(GifFile, + 0, 0, Width, Height, false, NULL) == GIF_ERROR) { + PrintGifError(Error); ++ free(OutputBuffer); ++ GifFreeMapObject(OutputColorMap); + exit(EXIT_FAILURE); + } + +@@ -187,8 +191,11 @@ static void SaveGif(GifByteType *OutputBuffer, + GifFile->Image.Width, GifFile->Image.Height); + + for (i = 0; i < Height; i++) { +- if (EGifPutLine(GifFile, Ptr, Width) == GIF_ERROR) ++ if (EGifPutLine(GifFile, Ptr, Width) == GIF_ERROR) { ++ free(OutputBuffer); ++ GifFreeMapObject(OutputColorMap); + exit(EXIT_FAILURE); ++ } + GifQprintf("\b\b\b\b%-4d", Height - i - 1); + + Ptr += Width; +@@ -196,6 +203,8 @@ static void SaveGif(GifByteType *OutputBuffer, + + if (EGifCloseFile(GifFile, &Error) == GIF_ERROR) { + PrintGifError(Error); ++ free(OutputBuffer); ++ GifFreeMapObject(OutputColorMap); + exit(EXIT_FAILURE); + } + } +-- +2.43.0 +
Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com> --- ...veral-defects-found-by-Coverity-scan.patch | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 package/giflib/0004-Fix-several-defects-found-by-Coverity-scan.patch