maintenance of NanoSVG #180

darealshinji · 2021-01-15T19:34:41Z

NanoSVG is no longer actively maintained.
Are there any plans to still fix some issues, such as memononen/nanosvg#136 ?

erco77 · 2021-01-15T20:05:51Z

On 1/15/21 11:34 AM, darealshinji wrote: NanoSVG is no longer actively maintained.

Source for this info? Last commit on the Nanosvg author's site was 2 months ago (Nov 19,2020) which doesn't sound like a dead project. Slow to respond maybe..

Are there any plans to still fix some issues, such as memononen/nanosvg#136 <memononen/nanosvg#136> ?

Normally for 3rd party packages we pull security fixes, but if any fltk dev or user provides a patch, I advise providing it to the upstream package _first_ to make sure it's a proper fix by the author if that's possible, then we can pull the fixes en masse as a security update before release.

erco77 · 2021-01-15T20:58:47Z

[EDIT: Updated]
Perhaps something along the lines of this might be an easy fix.. shorter, and drops the separate functions:

// [Rewrite of the nsvg__parseColor() function - erco 01/15/2021
//
// Handle the variations:
//     "rgb(255, 128, 64)"    -- 3 decimal integer rgb values (0-255)
//     "rgb(100%, 50%, 25%)"  -- 3 decimal integer rgb percentages (0-100)
//     "#6495ED"              -- 3 hex values as either single or two digit hex
//     "blue"                 -- color name
//
static unsigned int nsvg__parseColor(const char* str)
{
        unsigned int r=0, g=0, b=0;
        while(*str == ' ') ++str;
        if (sscanf(str, "#%2x%2x%2x", &r, &g, &b) == 3 )                // 2 digit hex
                return NSVG_RGB(r, g, b);
        else if (sscanf(str, "#%1x%1x%1x", &r, &g, &b) == 3 )           // 1 digit hex
                return NSVG_RGB(r*16, g*16, b*16); 
        else if (sscanf(str, "rgb(%u,%u,%u)", &r, &g, &b) == 3)         // decimal integers
                return NSVG_RGB(r, g, b);
        else if (sscanf(str, "rgb(%u%%,%u%%,%u%%)", &r, &g, &b) == 3)   // decimal integer percentage
                return NSVG_RGB(r*255/100, g*255/100, b*255/100); 
        return nsvg__parseColorName(str);
}

I didn't check how the SVG spec says to handle odd cases like negative values; I'm assuming positive unsigned for the decimal integers.

We can try to hash it out here, then I'll provide an upstream patch if it works out.

Attaching test code foo.cxx which compiles with e.g. g++ foo.cxx -o foo

Albrecht-S · 2021-01-16T00:18:08Z

NanoSVG is no longer actively maintained.

Source for this info?

README.md:

This project is not actively maintained.

Edit: ... since Apr 22, 2019: memononen/nanosvg@3e2a632

erco77 · 2021-01-16T00:46:11Z

Great -- then I guess we'll hash out a fix here then.

I see on the nanosvg site someone mentioned using google's OSS-Fuzz to search for bugs in the svg parser. Perhaps something we should look into. Luckily the code is not large, so a security audit would perhaps not be too hard.

Albrecht-S · 2021-01-16T00:46:21Z

Regarding the patch proposal: although there are only three cases mentioned in the comment, what about any combinations (w/o knowing anything about the specs) like: "rgb(100%, 255, 033)", and I do also think that

        else if (sscanf(str, "#%1x%1x%1x", &r, &g, &b) == 3 )           // 1 digit hex
                return NSVG_RGB(r*16, g*16, b*16);

is very likely not correct. Text like '#aaa' in html or css is AFAICT a shortcut for '#aaaaaa' and not '#a0a0a0' but don't know what it would mean in SVG context. Anybody?
FWIW, here's a CSS demo:

<!DOCTYPE html>
<html>
<head>
<style>
body {
  background-color: #a0b0c0; /* or use #aabbcc */
}
</style>
</head>
<body>

<h1>The background-color Property</h1>

<p>The background color can be specified with a color name.</p>
<p style='background-color: #abc';>New text<p>

</body>
</html>

Save it as a .html file or test it online.

Note the latter could be fixed easily by replacing (r*16, g*16, b*16) with (r*17, g*17, b*17) (if it's the same as in CSS).

Albrecht-S · 2021-01-16T00:52:35Z

Great -- then I guess we'll hash out a fix here then.

Note that we have already our own NanoSVG fork where I've been maintaining our own fixes/additions. See also our README.bundled-libs.txt (this note).

erco77 · 2021-01-16T01:00:44Z

Regarding our own nanoSVG fork, ah, OK.

For now I'll work patches in this issue, but at some point I'll move to that.

Here's a tweak for the 1 digit hex, so that e.g. #abc == 0xccbbaa (svg uses BGR ordering for their RGB encoding in unsigned ints).

        else if (sscanf(str, "#%1x%1x%1x", &r, &g, &b) == 3 )           // 1 digit hex
                return NSVG_RGB((r<<4|r), (g<<4|g), (b<<4|b));

Attaching a modified foo.cxx with that fix and an improved regression tester.
I've not addressed the mix of percent and integer values.. that would be a slightly move involved change, but can still be done mostly with sscanf(). My main goal is to avoid buffer overruns and such.

erco77 · 2021-01-16T01:06:24Z

Regarding #abc in SVG, it's apparently one of the allowed specs. I looked up the SVG spec for specifying colors, and allows for that and for percentages in floating point format, though nanosvg only supports integer percents from what I could tell, so I was sticking with that limitation. We could support floating percentage easily enough, that way a decimal point wouldn't cause havoc. Though if we go there, we'd have to be careful as some locales use different characters for decimal points. I didn't dig into the spec, but I assume all floating point numbers must be '.' (i.e. LANG=C) to avoid locale specific SVG files. And IIRC, sscanf() is affected by locale. Not sure what the right way is to deal with that, as changing the locale in the code would be bad for threads.

erco77 · 2021-01-16T01:22:51Z

@Albrecht-S just noticed your *16 -> *17 solution which sounds much better than my shift/OR, and does seem to work just as well. Revised foo.cxx attached which also does some extra regression testing for the 3 digit variations.

Albrecht-S · 2021-01-16T01:46:28Z

we'd have to be careful as some locales use different characters for decimal points. I didn't dig into the spec, but I assume all floating point numbers must be '.' (i.e. LANG=C) to avoid locale specific SVG files. And IIRC, sscanf() is affected by locale.

I have a vague recollection that NanoSVG has an own parser part/function that avoids such locale issues. I've been following the commits from time to time, hence that vague recollection...

Maybe this commit (Dec 14, 2018) regarding memononen/nanosvg#139 can give a hint? Basically replacing sscanf with nsvg__atof(str).

Albrecht-S · 2021-01-16T02:01:06Z

Regarding our own nanoSVG fork, ah, OK.
For now I'll work patches in this issue, but at some point I'll move to that.

I noticed that (as you mentioned, despite NanoSVG is "not actively maintained") there have been some commits and merges since the last commit (2019-05-23) I used in our fork. Looks as if memononen (the maintainer) and some other person still are doing some updates.

Before we commit to our fork we should thoroughly check if we can pull the upstream changes first, rebase our stuff, and then commit. Or commit to our branch, then pull the updates, and rebase later. Or something like that...

erco77 · 2021-01-16T02:35:30Z

Rather than try to mix code improvements in with a security fix, I'd just like to close out this issue by solving the OP's security issue (CVE-2019-1000032), for our users at least.

Note that what nanosvg's current code supports:

Integer-only percentages (e.g. rgb(100%,99%,98%), no floats), so no worries about decimal points/locale here
rgb() percentages can't be mixed with integer only rgb's; it's either all of one or all of the other.

So I think my patch is consistent with the current behavior and should solve the security issue.

My read of the svg color 1.2 spec from both the text and the Bakus Naur format spec you can either have integers or percentages in the rgb() spec, but not a mix, and all three percents are required when specified. So I think that's consistent with how my patch suggestion returns nanosvg's 0x808080 "error color" when there's any mix of the two.

erco77 · 2021-01-16T03:07:29Z

So here's an actual patch suggestion for our fltk/nanosvg fork.
Kept the original RGB and HEX functions to minimize the diff.
nanosvg-patch-v1.txt

Albrecht-S · 2021-01-17T01:04:32Z

@erco77 👍 Patch looks good, but I didn't test it (yet).

erco77 · 2021-01-17T03:37:27Z

I should probably make a test image in SVG that exercises all the possible rgb() variations, and add it as a regression tester. I could see having color bars with text in each explaining what color it should be (including error colors)

erco77 · 2021-01-17T15:30:32Z

Attaching test-rgb-syntax.svg.zip which contains:

test-rgb-syntax.svg -- hand crafted svg test image that is a starting point to test the rgb() syntax variations
example2.c -- a modified version of the original example2.c that allows an optional argument
Makefile -- builds example2, and has a 'make test' target to build/run/convert and load the result in gimp

Since nanosvg doesn't support text, had to present two columns of color tiles that can be visually compared.

There's two columns of color tiles: left and right. The left column is under test, right column is the expected color.
So the left and right columns should look exactly the same.

Where things will go into undefined territory (depends on the SVG reader's implementation) is the last block of rows, which test improper rgb() syntax handling. These columns won't necessarily match, depending on the SVG implementation. I've found that even firefox and gimp disagree on how parse these 'bad syntax' colors, so seems like anything goes for those. In the case of this test, the right column shows what I think the colors should be. Note the handling of 101% is "wrong"; I think it should clip, but nanosvg seems to wrap colors >255.

erco77 · 2021-01-18T23:08:15Z

Added a branch "fltk-issue-180" to fltk/nanosvg with the above patch, and addressed the handling of percentages >100 with a clip. To run the test: ( cd example ; make )

wcout · 2021-01-19T04:49:48Z

@erco77: Since nanosvg doesn't support text, had to present two columns of color tiles that can be visually compared.

You could try exporting text to SVG paths, which works quite well. For example you can use inkscape from the command line:

inkscape -l output.svg -T input.svg

erco77 · 2021-01-19T06:06:18Z

@wcout Thanks, it works in that inkscape translates the tags into strokes -- good to know! But in this case, using inkscape causes all the color strings under test to be regenerated into a constant syntax, which unfortunately defeats the purpose of the test svg to exercise the color string parser.

wcout · 2021-01-19T06:49:51Z

@erco77 : Oh yes, that was to be expected in this case :( So you could only merge/replace the converted text groups back to a copy of the original file, which seems quite tedious - would certainly require a script.

Albrecht-S · 2021-02-22T13:52:59Z

Summary:

@darealshinji To answer the main question: yes, we will update all our bundled libs (not only nanosvg) from time to time. Security patches like the one mentioned here may be applied by us directly (as done now).

@erco77 I updated our nanosvg fork with the latest upstream mods and applied your patch in our maintenance branch 'fltk' - only the code changes to the library, not the example progs. The latter is left in branch fltk-issue-180 for reference.

Edit: Note I force-pushed the new branch fltk because it was rebased.

Finally I committed the nanosvg updates to our FLTK lib (commit 9f84fd0).

I suggest to close this issue (label "fixed" applied).

erco77 · 2021-02-23T06:51:27Z

The OP never followed up and it's been over a month,, so yes, closing this.

darealshinji · 2021-02-24T07:47:23Z

yes, we will update all our bundled libs (not only nanosvg) from time to time.

Maybe you want to update libpng (1.6.37, 04-2019) and zlib (1.2.11, 01-2017) in the next time too.

Albrecht-S · 2021-02-24T08:34:23Z

Maybe you want to update libpng (1.6.37, 04-2019) and zlib (1.2.11, 01-2017) in the next time too.

Sure, that's my plan (and jpeg too, if new versions are available). The mentioned versions of libpng and zlib are up-to-date in 1.4.x (Git) but will be updated in the next release 1.3.6 [1].

libjpeg can be updated to 9d (12-Jan-2020) in 1.4.x and 1.3.6 [1].

[1] unless something unexpected prevents updating 1.3.6

darealshinji · 2021-09-12T23:51:36Z

NanoSVG was updated upstream to fix CVE-2019-1000032
memononen/nanosvg@ccdb199

Albrecht-S · 2021-09-13T17:43:17Z

@darealshinji Thanks for the heads-up.

FTR: I updated our own fork of nanosvg to include the latest upstream version (in branch master) and our own changes in branch fltk as usual. I noticed that our fork had an additional clipping of RGB percent values to <= 100 (RGB value 255) which I kept in our own fork/branch.

The result was committed in a774e12.

erco77 self-assigned this Jan 15, 2021

erco77 added the active Somebody is working on it label Jan 15, 2021

Albrecht-S changed the title ~~maintanance of NanoSVG~~ maintenance of NanoSVG Jan 16, 2021

Albrecht-S added fixed The issue or PR was fixed. and removed active Somebody is working on it labels Feb 22, 2021

erco77 closed this as completed Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maintenance of NanoSVG #180

maintenance of NanoSVG #180

darealshinji commented Jan 15, 2021

erco77 commented Jan 15, 2021 via email

erco77 commented Jan 15, 2021 •

edited

Loading

Albrecht-S commented Jan 16, 2021 •

edited

Loading

erco77 commented Jan 16, 2021

Albrecht-S commented Jan 16, 2021

Albrecht-S commented Jan 16, 2021

erco77 commented Jan 16, 2021

erco77 commented Jan 16, 2021

erco77 commented Jan 16, 2021 •

edited

Loading

Albrecht-S commented Jan 16, 2021

Albrecht-S commented Jan 16, 2021

erco77 commented Jan 16, 2021 •

edited

Loading

erco77 commented Jan 16, 2021 •

edited

Loading

Albrecht-S commented Jan 17, 2021

erco77 commented Jan 17, 2021

erco77 commented Jan 17, 2021

erco77 commented Jan 18, 2021

wcout commented Jan 19, 2021

erco77 commented Jan 19, 2021

wcout commented Jan 19, 2021

Albrecht-S commented Feb 22, 2021 •

edited

Loading

erco77 commented Feb 23, 2021

darealshinji commented Feb 24, 2021

Albrecht-S commented Feb 24, 2021

darealshinji commented Sep 12, 2021

Albrecht-S commented Sep 13, 2021

maintenance of NanoSVG #180

maintenance of NanoSVG #180

Comments

darealshinji commented Jan 15, 2021

erco77 commented Jan 15, 2021 via email

erco77 commented Jan 15, 2021 • edited Loading

Albrecht-S commented Jan 16, 2021 • edited Loading

erco77 commented Jan 16, 2021

Albrecht-S commented Jan 16, 2021

Albrecht-S commented Jan 16, 2021

erco77 commented Jan 16, 2021

erco77 commented Jan 16, 2021

erco77 commented Jan 16, 2021 • edited Loading

Albrecht-S commented Jan 16, 2021

Albrecht-S commented Jan 16, 2021

erco77 commented Jan 16, 2021 • edited Loading

erco77 commented Jan 16, 2021 • edited Loading

Albrecht-S commented Jan 17, 2021

erco77 commented Jan 17, 2021

erco77 commented Jan 17, 2021

erco77 commented Jan 18, 2021

wcout commented Jan 19, 2021

erco77 commented Jan 19, 2021

wcout commented Jan 19, 2021

Albrecht-S commented Feb 22, 2021 • edited Loading

erco77 commented Feb 23, 2021

darealshinji commented Feb 24, 2021

Albrecht-S commented Feb 24, 2021

darealshinji commented Sep 12, 2021

Albrecht-S commented Sep 13, 2021

erco77 commented Jan 15, 2021 •

edited

Loading

Albrecht-S commented Jan 16, 2021 •

edited

Loading

erco77 commented Jan 16, 2021 •

edited

Loading

erco77 commented Jan 16, 2021 •

edited

Loading

erco77 commented Jan 16, 2021 •

edited

Loading

Albrecht-S commented Feb 22, 2021 •

edited

Loading