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

6 bugs found in sam2p #14

Closed
ghost opened this issue Sep 18, 2017 · 8 comments
Closed

6 bugs found in sam2p #14

ghost opened this issue Sep 18, 2017 · 8 comments

Comments

@ghost
Copy link

ghost commented Sep 18, 2017

bug 1: integer-overflow(lead to heap-buffer-overflow)
poc: https://drive.google.com/open?id=0B4aWmtdznlVKaXJKbU13cTBCTTg
asan: https://drive.google.com/open?id=0B4aWmtdznlVKbkE1dEloaGdudms
method: ./sam2p crash EPS: /dev/null
reason:

pinfo->w = (hdr[PCX_XMAXL] + ((int) hdr[PCX_XMAXH]<<8))

after the subduction, w will be a negative number.
else pad = bperlin - w;

so pad will become larger when the program use it to sub w.
image -= pad;

and when using pad, it will access the invalid memory before the allocated chunk.
patch: change w from signed to unsigned.

bug 2: heap-buffer-overflow
poc: https://drive.google.com/open?id=0B4aWmtdznlVKYzhJZjVZbDhBbG8
asan: https://drive.google.com/open?id=0B4aWmtdznlVKelhYUFFGOGRKaDQ
method: ./sam2p crash EPS: /dev/null
reason:

static int pcxLoadImage24 ___((char *fname, FILE *fp, PICINFO *pinfo, byte *hdr), (fname, fp, pinfo, hdr),

The crash happened in the pcxLoadImage24 function of the file in_pcx.cpp. The size of the Pic24 is w * h * planes.
But the loop time is nbytes=bperlinhplanes, in the loop, pix will add one each time.
It will cause a heap overflow when bperlin>w.
In this poc, bperlin=0x320,w=0x300.
patch: Compare the size of w and bperlin

bug 3: integer-overflow
poc: https://drive.google.com/open?id=0B4aWmtdznlVKT2pDeU5rdlV4RlU
asan: https://drive.google.com/open?id=0B4aWmtdznlVKb05nWTlkZlIwUm8
method: ./sam2p crash EPS: /dev/null
reason:
It crashed in function in_xpm_reader. The type of p is char*
When p[0]=0xa0,p[1]=0x20, p[0] will be recognized as a negative integer, it will make bin[0xffffffffffffa020]=I, which caused a crash.
patch: change p from signed to unsigned char*.

bug 4: integer-overflow
poc: https://drive.google.com/open?id=0B4aWmtdznlVKUGttRHhDU0REMjQ
asan: https://drive.google.com/open?id=0B4aWmtdznlVKNzBJazNId1RHYzg
method: ./sam2p crash EPS: /dev/null
reason:

sam2p/image.cpp

Line 751 in ac4dee3

void Image::Indexed::sortPal() {

In this poc, when getNcols returned 0 to ncols, --ncols made it to 0xffffffff, and caused a integer overflow. So the loop will excute 0xffffffff times, which caused the crash.
patch: Define Ncols as an int, or you can judge if ncols is NULL.

bug 5: out-of-bounds access
poc: https://drive.google.com/open?id=0B4aWmtdznlVKSUpaODdRN2w5ekE
asan: https://drive.google.com/open?id=0B4aWmtdznlVKYUxsSnVLUGNwa2c
method: ./sam2p crash EPS: /dev/null
reason:

p=xpmColors_dat+xpmColors_ofs[(v&65535)%xpmColors_mod];

In in_xpm.cpp's function parse_rgb(),xpmcolors_mod = 1109, but the size of xpmColors_ofs is 1098.
If v&65535%xpmColors_mod > 1098 , it will crash.
patch:
modified xpmColors_mode or expand the xpmColors_ofs.

bug 6: integer-overflow
poc: https://drive.google.com/open?id=0B4aWmtdznlVKVFgzSjNMdkJyV00
asan: https://drive.google.com/open?id=0B4aWmtdznlVKc1J3MS1LUmVBekU
method: ./sam2p crash EPS: /dev/null
reason:

pic24 = (byte *) malloc_byte((PCX_SIZE_T) w*h*planes);

In in_pcx.cpp's function pcxLoadImage24, Whplanes will cause integer-overflow.
patch:
check Whplanes' value before malloc.

@ghost
Copy link
Author

ghost commented Sep 18, 2017

bug3 add:

} else if (cpp==2 && colors<=256) { /* Similarly easy job: make an Indexed image; defer .packPal() */

@pts
Copy link
Owner

pts commented Sep 18, 2017

Thank you very much for finding and reporting these bugs, your contribution is very much appreciated! I will fix the bugs based on your recommendations, and report progress on this issue.

@fgeek
Copy link

fgeek commented Sep 21, 2017

@fpbibi It would be easier to coordinate issues if you reported one case per issue report. Please consider doing so in the future, thanks.

@ghost
Copy link
Author

ghost commented Sep 22, 2017

Ok, I'm sorry about that. @fgeek I will split bugs into different issues next time.

@ghost
Copy link
Author

ghost commented Sep 22, 2017

@fgeek @pts Hello~~ It's my pleasure to add informations about bug4 and bug5

bug 4:

In this fuction:

sam2p/image.cpp

Line 751 in ac4dee3

void Image::Indexed::sortPal() {

when getNcols returned 0 to ncols

sam2p/image.cpp

Line 753 in ac4dee3

unsigned ncols = getNcols(), i;

--ncols made it to 0xffffffff, and caused a integer overflow.

sam2p/image.cpp

Line 756 in ac4dee3

if (transp + 0U == ncols - 1) --ncols;

So the loop will execute 0xffffffff times:

sam2p/image.cpp

Line 767 in ac4dee3

for (i = 0, p = (unsigned char*)headp; i < ncols; ++i, p += 3) {

In the for(......), the parameter d is on the stack:

sam2p/image.cpp

Line 768 in ac4dee3

d[i] = (d_t)p[0] << 24 | (d_t)p[1] << 16 | (d_t)p[2] << 8 | i;

when the parameter ncols is 0xffffffff, d[i] will be an address on the stack.So it caused an attribute write to this address, which may lead to code execution. Thats why the program received a segment fault.

picture for segment fault:
bug4_segment_fault

firstly, getNcols returned 0 to ncols:
3

--ncols made it to 0xffffffff
4

in for(......), the parameter d is on the stack:
5

i: from 0 to 0xffffffff, and we can write stack 0xffffffff times:

sam2p/image.cpp

Line 768 in ac4dee3

d[i] = (d_t)p[0] << 24 | (d_t)p[1] << 16 | (d_t)p[2] << 8 | i;

6

@ghost
Copy link
Author

ghost commented Sep 22, 2017

@pts @fgeek
bug 5:

p=xpmColors_dat+xpmColors_ofs[(v&65535)%xpmColors_mod];

default

Here, the address of xpmColors_ofs is an global array on the .bss area of the program, and when the program calculate the offset (v&65535)%xpmColors_mod, the answer is 0x450.
So xpmColors_ofs[(v&65535)%xpmColors_mod ] will beyond the original size 1098, it will be 0xfa1e here.

default

xpmColors_dat is on the bss. So p = xpmColors_dat+xpmColors_ofs[(v&65535)%xpmColors_mod] will be an illegal address.

default

If we designed the xpmColors_ofs[(v&65535)%xpmColors_mod] carefully.We can do attribute write.

@pts
Copy link
Owner

pts commented Sep 27, 2017

Fixed all these bugs and created a new binary release: https://github.com/pts/sam2p/releases/tag/v0.49.4

@pts pts closed this as completed Sep 27, 2017
pts pushed a commit that referenced this issue Sep 27, 2017
pts pushed a commit that referenced this issue Sep 27, 2017
pts pushed a commit that referenced this issue Sep 27, 2017
pts pushed a commit that referenced this issue Sep 27, 2017
pts pushed a commit that referenced this issue Sep 27, 2017
pts pushed a commit that referenced this issue Sep 27, 2017
pts pushed a commit that referenced this issue Oct 23, 2017
pts pushed a commit that referenced this issue Oct 23, 2017
pts pushed a commit that referenced this issue Oct 23, 2017
pts pushed a commit that referenced this issue Oct 23, 2017
pts pushed a commit that referenced this issue Oct 23, 2017
pts pushed a commit that referenced this issue Oct 23, 2017
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

2 participants