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

Out-of-bounds memory access in DXF loader (path identification) #4037

Closed
eldstal opened this issue Jan 5, 2022 · 9 comments
Closed

Out-of-bounds memory access in DXF loader (path identification) #4037

eldstal opened this issue Jan 5, 2022 · 9 comments

Comments

@eldstal
Copy link
Contributor

eldstal commented Jan 5, 2022

Summary

A DXF-format drawing with particular (not necessarily malformed!) properties may cause an out-of-bounds memory access when imported using import().

Vulnerable versions

  • OpenSCAD Linux (commit eedf370)
  • OpenSCAD Windows x64 (2021.01)

Steps to reproduce

Two Proof-of-concept files are provided. oob_dxfdata_505 is larger, but more reliably reproduces the fault. oob_dxfdata_k5 is my attempt at a minimal working example, but only reliably crashes the linux build from the latest commit on the main branch (i.e. it does not crash in the windows release). Both illustrate the same crash, so I expect the _min example to be more helpful in testing.

oob_dxfdata_505.zip, the larger messier p-o-c
oob_dxfdata_k5.zip, the smaller p-o-c

  1. Unzip one of the provided proof of concept files
  2. Open the .scad file in OpenSCAD
  3. Render with F6
  4. Observe the application crashing

Alternatively, for headless operation:

  1. Unzip one of the provided proof of concept files
  2. openscad --export-format stl -o /dev/null oob_dxfdata_k5.scad
  3. Observe the segmentation fault

Screenshot

image

Cause

I haven't quite been able to wrap my head around the DXF parser yet, but the OOB access occurs in src/dxfdata.cc on one of the lines 470, 475, 505, or 510 depending on the input file.

It appears to be related to the fact that if multiple line segments share points in common, they are merged into contiguous paths. As a part of this, The ADD_LINE macro manipulates the grid.data<> and lines<> structures. By creating lines either in the ENTITIES section of the DXF file or outside of it, an attacker is able to ensure that the grid.data entry created on line 108 points to an index in lines which never becomes valid.

On line 470 (and the others listed above), this value (k) is used as an index into lines, out of bounds.

Impact

It appears that the out-of-bounds data can only be read, and not written. An out-of-bounds read does not expose a security vulnerability on its own, but can be used to bypass automatic security features such as stack canaries and pointer encryption.

Proposed mitigation

The algorithm employed in DxfData::DxfData needs to be revised to prevent this type of aliasing. Without more insight into the DXF format and how it is used by OpenSCAD, I cannot say for sure where the actual flaw lies.

It seems likely that the ADD_LINE macro on lines 108-109 is the culprit, since it inserts values that will be used for indexing even though the values are not yet valid indices.

@ChrisCoxArt
Copy link
Contributor

ChrisCoxArt commented Jan 16, 2022

I see lots of warnings, but no crash on MacOS with the current development code.
I even tried with guard malloc, guard edges, and zombie (use after free) detection enabled.
Ah, but got it to fail with malloc_scribble enabled!
It's reading one entry over the end of the lines vector (size = 10, index = 10). Because of the way vectors double allocations (so this one has 16 values allocated), the address is valid, but the data is bogus, and the data is used as an index to another array, which then gets the access violation. Under normal debugging situations, the memory was probably zero-ed and leading to a valid index.

@ChrisCoxArt
Copy link
Contributor

ChrisCoxArt commented Jan 16, 2022

And there are at least 3 places in the code with the same fragility.
OK, probably best to skip any bad indices, and log a warning about the bad index in the DXF file.

I hope this doesn't break any import tests... Good, it doesn't break any of the tests. I still want to test this with more files, though.

@ChrisCoxArt
Copy link
Contributor

ChrisCoxArt commented Jan 16, 2022

I thought maybe we could catch the error earlier in the process (when adding lines), but so far I haven't figured out a way to do it.
This DXF code is more than a little fragile.

BTW - the second example actually hits the same sort of error in a different place, so thanks for providing both!

@kintel
Copy link
Member

kintel commented Jan 16, 2022

OpenSCAD's DXF support is really a rather large piece of technical debt. At some point it's worth digging further into replacing it with an existing library, see https://github.com/openscad/openscad/wiki/Project%3A-Improve-DXF-import-and-export

@ChrisCoxArt
Copy link
Contributor

Ok, I have a fix that is safe, if not optimal. Trying to catch the error earlier runs into a lot of code that I cannot decipher easily (it's a huge state machine, with many inter-dependent arrays of data) -- so I cannot say that any changes there would be safe.

@eldstal
Copy link
Contributor Author

eldstal commented Feb 4, 2022

Thanks for taking a look at this issue! Has the fix been merged?

This vulnerability has been assigned CVE-2022-0496.

@ChrisCoxArt
Copy link
Contributor

No, the fix has not been merged. It's waiting on other pull requests to be merged (which have been sitting for 3 weeks).

ChrisCoxArt added a commit to ChrisCoxArt/openscad that referenced this issue Feb 4, 2022
Add safety (test for, and continue past, bad indices).
Report warnings about bad indices
Add variables just to make the array indices easier to read and debug.
@ChrisCoxArt
Copy link
Contributor

Now it has been merged! yay!

@t-paul
Copy link
Member

t-paul commented Feb 4, 2022

Hold the horses, it will be in a bit :)

@t-paul t-paul closed this as completed in 770e323 Feb 5, 2022
t-paul added a commit that referenced this issue Feb 5, 2022
Add safety to line lookups in DXF import (fixes #4037).
t-paul added a commit that referenced this issue Feb 5, 2022
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

4 participants