Re: [X2Go-Dev] [X2Go-Commits] [nx-libs] 28/52: CVE-2014-0210: unvalidated length fields in fs_read_list() from xorg/lib/libXfont commit 5fa73ac18474be3032ee7af9c6e29deab163ea39
On 14.02.2015 05:47 PM, git-admin@x2go.org wrote:
This is an automated email from the git hooks/post-receive script.
x2go pushed a commit to branch 3.6.x in repository nx-libs.
commit ef439da38d3a4c00a4e03e7d8f83cb359cd9a230 Author: Mike DePaulo <mikedep333@gmail.com> Date: Sun Feb 8 22:35:21 2015 -0500
CVE-2014-0210: unvalidated length fields in fs_read_list() from xorg/lib/libXfont commit 5fa73ac18474be3032ee7af9c6e29deab163ea39 fs_read_list() parses a reply from the font server. The reply contains a list of strings with embedded length fields, none of which are validated. This can cause out of bound reads when looping over the strings in the reply.
nx-X11/lib/font/fc/fserve.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/nx-X11/lib/font/fc/fserve.c b/nx-X11/lib/font/fc/fserve.c index 26218e5..60d9017 100644 --- a/nx-X11/lib/font/fc/fserve.c +++ b/nx-X11/lib/font/fc/fserve.c @@ -2365,6 +2365,7 @@ fs_read_list(FontPathElementPtr fpe, FSBlockDataPtr blockrec) FSBlockedListPtr blist = (FSBlockedListPtr) blockrec->data; fsListFontsReply *rep; char *data;
- long dataleft; /* length of reply left to use */
Same here. long dataleft = 0;
int length, i, ret;@@ -2382,16 +2383,30 @@ fs_read_list(FontPathElementPtr fpe, FSBlockDataPtr blockrec) return AllocError; } data = (char *) rep + SIZEOF (fsListFontsReply);
dataleft = (rep->length << 2) - SIZEOF (fsListFontsReply);
err = Successful; /* copy data into FontPathRecord */ for (i = 0; i < rep->nFonts; i++) {
if (dataleft < 1)
break;
Just as a heads-up: I would have moved this into the for loop condition like so: for (i = 0; (i < rep->nFonts) && (dataleft > 0); i++) to make clear, that it's really part of the looping condition. The current patch as provided by upstream is functionally equivalent and OK, though. Everything else in the patch looks good.
On Sun, Feb 15, 2015 at 10:02 PM, Mihai Moldovan <ionic@ionic.de> wrote:
Just as a heads-up: I would have moved this into the for loop condition like so:
The code might offer a lot of possibilities for improvement. However, as all this is derived from the original X11 code I would prefer leaving it as is (and fix it upstream). This will make it a lot easier to backport later patches and it will also make the nx transition to current X11 much easier. Maybe add FIXME: comments to not forget those ideas.
Uli
On Mon, Feb 16, 2015 at 3:29 AM, Ulrich Sibiller <ulrich.sibiller@gmail.com> wrote:
On Sun, Feb 15, 2015 at 10:02 PM, Mihai Moldovan <ionic@ionic.de> wrote:
Just as a heads-up: I would have moved this into the for loop condition like so:
The code might offer a lot of possibilities for improvement. However, as all this is derived from the original X11 code I would prefer leaving it as is (and fix it upstream). This will make it a lot easier to backport later patches and it will also make the nx transition to current X11 much easier. Maybe add FIXME: comments to not forget those ideas.
Uli
+1
On 16.02.2015 09:29 AM, Ulrich Sibiller wrote:
Just as a heads-up: I would have moved this into the for loop condition like so: The code might offer a lot of possibilities for improvement. However, as all this is derived from the original X11 code I would prefer leaving it as is (and fix it upstream). This will make it a lot easier to backport later patches and it will also make the nx transition to current X11 much easier. Maybe add FIXME: comments to not forget
On Sun, Feb 15, 2015 at 10:02 PM, Mihai Moldovan <ionic@ionic.de> wrote: those ideas.
Uli
You're right. That's why I have only changed initialization where conflicts are easily merged and the 1 MB thing.
Everything else was left in place. I'm just bringing it up so that people don't follow (bad) examples.
Mihai
On Mon, Feb 16, 2015 at 8:23 PM, Mihai Moldovan <ionic@ionic.de> wrote:
The code might offer a lot of possibilities for improvement. However, as all this is derived from the original X11 code I would prefer leaving it as is (and fix it upstream). This will make it a lot easier to backport later patches and it will also make the nx transition to current X11 much easier. Maybe add FIXME: comments to not forget those ideas.
You're right. That's why I have only changed initialization where conflicts are easily merged and the 1 MB thing.
Everything else was left in place. I'm just bringing it up so that people don't follow (bad) examples.
This brings up the question: Should we try to backport any bugfixes? Or should we skip that completely and concentrate on rebasing nx to current X11 (Mike is working on that). Or should we do both in parallel?
Uli
Hi Uli,
On Mo 16 Feb 2015 21:29:56 CET, Ulrich Sibiller wrote:
On Mon, Feb 16, 2015 at 8:23 PM, Mihai Moldovan <ionic@ionic.de> wrote:
The code might offer a lot of possibilities for improvement. However, as all this is derived from the original X11 code I would prefer leaving it as is (and fix it upstream). This will make it a lot easier to backport later patches and it will also make the nx transition to current X11 much easier. Maybe add FIXME: comments to not forget those ideas.
You're right. That's why I have only changed initialization where conflicts are easily merged and the 1 MB thing.
Everything else was left in place. I'm just bringing it up so that people don't follow (bad) examples.
This brings up the question: Should we try to backport any bugfixes? Or should we skip that completely and concentrate on rebasing nx to current X11 (Mike is working on that). Or should we do both in parallel?
This heavily depends, I feel.
For CVEs, I am so happy about Mike#2's work on the CVE audit.
For other issues, I think we should fix bugs that hit us heavily while
at the same time working on replacing this and that portion of code in
nx-libs (or making nxagent dynamically link against X.Org's libX*
libraries).
I have now started with an imake-cleanup branch at [1]. I still feel
that we should work on kicking out everything non-used before starting
with the rebase work. I really don't want to work on rebasing stuff
that will get caught by rm -Rf two days later.
However, there are also some obvious parts that are definitely used,
so here we could work on in parallel.
Some ideas:
o update xrandr to match protocol version 1.4 o replace linking against libNX_Xdmcp by linking against libXdmcp o maybe replacing linkage for other libNX_X* libraries (e.g. libXext? Xcomposite? Xdamage? Xtst?):
Here is an overview of Xlibs that got patched by NoMachine and/or X2Go
regarding their functionality:
font
GL
SM
libNX_X11
libNX_Xau
libNX_Xinerama (changed by X2Go)
libNX_Xrandr
libNX_Xpm (only Imakefile)
libNX_Xrender
libNX_Xt (linked statically)
xtrans (linked statically)Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, herweg 7, 24357 fleckeby fon: +49 (1520) 1976 148
GnuPG Key ID 0x25771B31 mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
-
Michael DePaulo -
Mihai Moldovan -
Mike Gabriel -
Ulrich Sibiller -
Ulrich Sibiller