Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

endless loop (DOS) when parsing crafted input via nsvgParseFromFile() #178

Closed
invd opened this issue Aug 1, 2020 · 13 comments
Closed

endless loop (DOS) when parsing crafted input via nsvgParseFromFile() #178

invd opened this issue Aug 1, 2020 · 13 comments

Comments

@invd
Copy link

invd commented Aug 1, 2020

Mid-June, I discovered and privately reported an endless loop issue that happens in the following usage scenario with a small crafted input file:

#include <stdint.h>
#include <stdio.h>

#define NANOSVG_IMPLEMENTATION
#include "nanosvg.h"

int main(int argc, char *argv[]) {
  // this will loop for some inputs
  NSVGimage* g_image = nsvgParseFromFile("nanosvg_loop_examplefile1.svg", "px", 96);
  return 0;
}

The expected security impact is a denial of service.

So far, I have not received a reply from @memononen. Given that nanosvg is not actively maintained (see README.md), this is somewhat expected, but I want to report the issue anyway because there might still be active users of this library that are affected by this.

@memononen : can you give some quick feedback on whether you want the details to be disclosed publicly here in the bugtracker or prefer them to stay nonpublic until the 15.9.2020 (90 days after initial disclosure)?

@tesch1
Copy link

tesch1 commented Aug 10, 2020

If you want to find some more to not disclose there's a fuzzer in one of the many branches. Should be totally obvious, but I'll point out that nobody security conscious should be using this library to load svgs from unknown sources.

@invd
Copy link
Author

invd commented Aug 10, 2020

Hello @tesch1, I'm not sure that I fully understood what you were trying to say.

As far as I can see, memononen/nanosvg has no other branches, active or otherwise. Looking deeper into the topic of actual nanosvg forks, I can find your comment at 25241c5#commitcomment-35986042 and your nanosvg fork, which does have fuzzing-related changes.

Could you please clarify what you're suggesting for the disclosure of the DOS mentioned above? In particular, do you think that you've already found and publicly fixed the relevant issue in your fork, which would resolve this topic?

Correctly disclosing issues in unmaintained projects with unreachable developers is difficult, and it doesn't help that there is no visible followup project with either the blessing of the previous maintainer or many visible users (measured by Github stars, for example). Given the number of stars and forks for this original version, it is not unreasonable to assume that this code is still used somewhere to parse untrusted input. Through this Github issue, I was trying to balance the relevant pros and cons of this situation.

@oehhar
Copy link
Contributor

oehhar commented Aug 17, 2020

The Tk project (Tcl/Tk (https://core.tcl-lang.org/tk), TkInter, TkRuby, TkPerl) are happily using svgnano and it is very helpful and very quick.
We also have a maintainer issue.

Some bugs are only fixed in Tk and not necessarily reported back to the main project.

We would be glad to get more information on what is actually triggering the issue.

Thank you,
Harald

@invd
Copy link
Author

invd commented Aug 17, 2020

Interesting, so the tksvg extension and the main tcl code also carry a copy of nanosvg.h after TIP #507 in 2018, or at least some branches do.
@oehhar Can you point me to a specific security contact of the Tcl/Tk project or member of the Tcl core team that is responsible for security matters so that we can discuss this privately?

@invd
Copy link
Author

invd commented Aug 23, 2020

@oehhar, @auriocus : I'm able to reproduce the issue with the nanosvg.h variant that is shipped in the latest version of auriocus/tksvg . I will contact you via email for further discussion.

@invd
Copy link
Author

invd commented Sep 15, 2020

Here is the nanosvg_loop_examplefile1.svg file that can be used to verify the issue:
nanosvg_loop_examplefile1.svg.zip

21 bytes are sufficient to trigger the problem:

00000000  3c 67 20 74 72 61 6e 73  66 6f 72 6d 3d 22 73 6b  |<g transform="sk|
00000010  65 77 58 28 31 20 31 29  3e                       |ewX(1 1)>|

@fvogelnew1
Copy link
Contributor

fvogelnew1 commented Sep 16, 2020

Quick fix proposal in Tk here

@oehhar
Copy link
Contributor

oehhar commented Sep 21, 2020

Here is the patch authored by fvogel:

--- generic/nanosvg.h
+++ generic/nanosvg.h
@@ -1668,29 +1668,36 @@
 }
 
 static void nsvg__parseTransform(float* xform, const char* str)
 {
 	float t[6];
+        int len;
 	nsvg__xformIdentity(xform);
 	while (*str)
 	{
 		if (strncmp(str, "matrix", 6) == 0)
-			str += nsvg__parseMatrix(t, str);
+			len = nsvg__parseMatrix(t, str);
 		else if (strncmp(str, "translate", 9) == 0)
-			str += nsvg__parseTranslate(t, str);
+			len = nsvg__parseTranslate(t, str);
 		else if (strncmp(str, "scale", 5) == 0)
-			str += nsvg__parseScale(t, str);
+			len = nsvg__parseScale(t, str);
 		else if (strncmp(str, "rotate", 6) == 0)
-			str += nsvg__parseRotate(t, str);
+			len = nsvg__parseRotate(t, str);
 		else if (strncmp(str, "skewX", 5) == 0)
-			str += nsvg__parseSkewX(t, str);
+			len = nsvg__parseSkewX(t, str);
 		else if (strncmp(str, "skewY", 5) == 0)
-			str += nsvg__parseSkewY(t, str);
+			len = nsvg__parseSkewY(t, str);
 		else{
 			++str;
 			continue;
 		}
+                if (len != 0) {
+			str += len;
+                } else {
+			++str;
+			continue;
+                }
 
 		nsvg__xformPremultiply(xform, t);
 	}
 }

memononen added a commit that referenced this issue Sep 21, 2020
Ticket #178: endless loop (DOS) when parsing crafted input via nsvgPa…
@invd
Copy link
Author

invd commented Sep 24, 2020

The merged patch appears to resolve the loop issue 👍

@memononen: additional fuzzing brings up more issues. Please indicate how you want those to be reported.
If I don't receive any feedback from you then I will report them in 2 weeks via public Github issue tickets in this repository.

@memononen
Copy link
Owner

memononen commented Sep 25, 2020

@invd please report them as you find them, I'll try to find time to merge PRs if they surface.
@oehhar Do you have time/resources/motivation to look into invd's findings?

@oehhar
Copy link
Contributor

oehhar commented Sep 25, 2020

Tierve Mikko,
thank you for the great project. It serves each day and makes applications and live beautiful.
Thanks, invd. I appreciate to take care about security.
I am more the messanger.
The tk community (core.tcl.tk/tk) member fvogel had solved the first ticket.
I will try again to support the process and bring issues and solutions from one world to the other.

Thank you,
Harald

memononen added a commit that referenced this issue Sep 28, 2020
- make sure nsvg__addPath() hands only valid number of pointts (1+N*3)
- require moveTo path command before handling other commands
- require (sign+)digit for a valid path command coordinate
- allow to add bezier segment only after there’s at leat one point (now
also consistent with nsvg__lineTo)
@memononen
Copy link
Owner

memononen commented Sep 28, 2020

Thanks Harald. I happened to have few hours to spare, so I fixed the to latest findings from @invd

@invd
Copy link
Author

invd commented Nov 28, 2020

I think this issue can be closed.

@invd invd closed this as completed Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants