Browse files

Fix format string vulnerability in using agerr() to report errors dur…

…ing parsing.

We now use a fixed format %s, and pass the error string as an argument.
  • Loading branch information...
1 parent faf196c commit 99eda421f7ddc27b14e4ac1d2126e5fe41719081 @emdenrg emdenrg committed Nov 24, 2014
Showing with 2 additions and 1 deletion.
  1. +2 −1 lib/cgraph/scan.l
@@ -225,6 +225,7 @@ ID ({NAME}|{NUMBER})
<hstring>([^><\n]*) addstr(yytext);
. return (yytext[0]);
void yyerror(char *str)
unsigned char xbuf[BUFSIZ];
@@ -273,7 +274,7 @@ void yyerror(char *str)
agxbputc (&xb, '\n');
- agerr(AGERR,agxbuse(&xb));
+ agerr(AGERR, "%s", agxbuse(&xb));
/* must be here to see flex's macro defns */

4 comments on commit 99eda42

Frodox commented on 99eda42 Dec 1, 2014

How to exploit this "format string vulnerability" with old versions ?


In case anyone is wondering how to cherry-pick this to 2.38 (as I was), I found a patch from Ubuntu at this Debian bug.

Note: I'm not a graphviz developer; this is not authoritative :-)

thoger commented on 99eda42 May 19, 2015

Is there any known way to make yytext contain the format string?

I see it's possible to inject format string via file name and (after dc6f1e0) via unclosed qstring or hstring, but I've not got to get it to yytext.

Quick grep over sources found few other agerr() calls with second argument not being a fixed string.
Same file, chkNum() function. Format string can be injected via graph file name:

$ cat 
graph G { a [ weight = 0g ] }

$ dot 
Warning: *** %n in writable segment detected ***
libgraph is deprecated since 2.30.0. In older graphviz versions, format string can be injected via graph file content.
This case would require format string in yytext, so it's unclear to me if it's possible or not.

Please sign in to comment.