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

A null pointer dereference bug of swfrender #46

Open
ghost opened this issue Nov 9, 2017 · 7 comments
Open

A null pointer dereference bug of swfrender #46

ghost opened this issue Nov 9, 2017 · 7 comments

Comments

@ghost
Copy link

ghost commented Nov 9, 2017

A null pointer dereference bug of swfrender
poc: https://drive.google.com/open?id=1GnHnojAXyZuAC-KkUIvRZTWFc0Popv3i
asan: https://drive.google.com/open?id=1wLHRbskl0CQPXA6IkLH01pP2dbhlWyIK

the problem is in the function uncompress

uncompress(data, &datalen, &tag->data[tag->pos],

when function uncompress failed, this function will return 0

if (error != Z_OK) {

so the *swf_ExtractImage will return 0.

https://github.com/matthiaskramm/swftools/blob/master/lib/readers/swf.c#L405

data will be a NULL when construct it.
void *data = swf_ExtractImage(tag, &width, &height);
After that, gfximage_new will make a new struct in which the data is a NULL pointer.
it caused a NULL pointer reference when calling fill_line_bitmap

col = b->data[yy*b->width+xx];

@ghost
Copy link
Author

ghost commented Nov 9, 2017

@ghost
Copy link
Author

ghost commented Nov 9, 2017

Patch Suggestion:
add an assert to check parameter data, if the data is NULL before calling gfximage_new
throw an exception.

@NicoleG25
Copy link

NicoleG25 commented Jan 8, 2020

Do you plan to address this vulnerability? :)
Note that CVE-2017-16711 was assigned
@yoya

@yoya
Copy link
Contributor

yoya commented Jan 8, 2020

swf_JPEG2TagToImage extracts an image from a JPEG2 / JPEG3 tag and returns it as an RGBA array.
JPEG3 tag include JPEGdata and zlib compressed Alphadata.
If Alphadata zlib inflating aborts with the error, the RGBA array is returned as NULL, and users of the RGBA array reference it without NULL checking, causing a segmentation fault.

First, it's wrong to return a NULL RGBA array for Alphadata inflation failed. Removing line "return 0;"(=NULL) allows to continue the process and use the partially inflated Alphadata without wasting previously decoded JPEG data.

At this point, CVE-2017-16711 will be superficially resolved.

Second, If we want to be more safety, we need to take care of those code reference the RGBA array without checking for NULL.

@yoya
Copy link
Contributor

yoya commented Jan 8, 2020

I'm sorry. The problem pointed out by CVE-2017-16711 is swf_DefineLosslessBitsTagToImage.
swf_DefineLosslessBitsTagToImage also has issues similar to swf_JPEG2TagToImage.
If inflation of zlib compressed RGBA array fails, return NULL as RGBA array.
In this case as well, we can deal with the same problem by deleting one line of "return 0;" and continuing the process.

@matthiaskramm
Copy link
Owner

Thanks for analyzing these, Yoya!

Any chance you could send me a pull request?

yoya added a commit to yoya/swftools that referenced this issue Jan 8, 2020
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-16711

Code modification to continue processing even if zlib infrate of image fails.

The current code returns 0(NULL) as an RGBA array when the zlib infrate of the image fails, so a segmentation fault occurs at the location that references it.

ref) matthiaskramm#46
matthiaskramm pushed a commit that referenced this issue Jan 8, 2020
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-16711

Code modification to continue processing even if zlib infrate of image fails.

The current code returns 0(NULL) as an RGBA array when the zlib infrate of the image fails, so a segmentation fault occurs at the location that references it.

ref) #46
@yoya
Copy link
Contributor

yoya commented Jan 11, 2020

sorry. PR69 was not enough care.
additional PR: #71

#69
the behavior has been changed so that processing continues even if inflation is incomplete.
With rfx_alloc(malloc wrapper), the possibility of displaying uninitialized data remains.
Should be changed to rfx_calloc(calloc wrapper).

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

3 participants