Skip to content

Commit

Permalink
Fix format string vulnerability in using agerr() to report errors dur…
Browse files Browse the repository at this point in the history 
…ing parsing.

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

4 comments on commit 99eda42

@Frodox
Copy link

@Frodox Frodox commented on 99eda42 Dec 1, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@emden
Copy link
Collaborator

@emden emden commented on 99eda42 Dec 1, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://www.owasp.org/index.php/Format_string_attack

@falsifian
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@thoger thoger commented on 99eda42 May 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://github.com/ellson/graphviz/blob/31158b1/lib/cgraph/scan.l#L168
Same file, chkNum() function. Format string can be injected via graph file name:

$ cat fs4-%n%s%s%s%s%s%s.dot 
graph G { a [ weight = 0g ] }

$ dot fs4-%n%s%s%s%s%s%s.dot 
Warning: *** %n in writable segment detected ***
Aborted

https://github.com/ellson/graphviz/blob/31158b1/lib/graph/lexer.c#L463
https://github.com/ellson/graphviz/blob/31158b1/lib/graph/lexer.c#L469
https://github.com/ellson/graphviz/blob/31158b1/lib/graph/lexer.c#L472
libgraph is deprecated since 2.30.0. In older graphviz versions, format string can be injected via graph file content.

https://github.com/ellson/graphviz/blob/31158b1/cmd/tools/gmlscan.l#L130
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.