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

CVE-2017-14633: Don't allow for more than 256 channels #34

Closed
wants to merge 2 commits into from

Conversation

@agx
Copy link
Contributor

agx commented Oct 31, 2017

Otherwise

for(i=0;ichannels;i++){
/* the encoder setup assumes that all the modes used by any
specific bitrate tweaking use the same floor */
int submap=info->chmuxlist[i];

overreads later in mapping0_forward since chmuxlist is a fixed array of
256 elements max.

This fixes https://gitlab.xiph.org/xiph/vorbis/issues/2329 for me but I don't know much about vorbis so I might be wrong.

Otherwise

 for(i=0;i<vi->channels;i++){
      /* the encoder setup assumes that all the modes used by any
         specific bitrate tweaking use the same floor */
      int submap=info->chmuxlist[i];

overreads later in mapping0_forward since chmuxlist is a fixed array of
256 elements max.
@@ -588,7 +588,7 @@ int vorbis_analysis_headerout(vorbis_dsp_state *v,
oggpack_buffer opb;
private_state *b=v->backend_state;

if(!b||vi->channels<=0){
if(!b||vi->channels<=0||vi->channels>256){

This comment has been minimized.

Copy link
@rillian

rillian Oct 31, 2017

Contributor

This should be > 255 I think?

Do you have a way for vi->channels to fail this check? The value is initialized from an one-byte read in the info packet data, so memory corruption is the only way I see for a value >255 to occur.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Nov 1, 2017

@rillian According to backend.h we can have 256 elements:
int chmuxlist[256]
and the check is for < vi-> channels so 0..255 is o.k.

I see the bug during encoding:

https://gitlab.xiph.org/xiph/vorbis/issues/2329

which uses vorbis_encode_setup_setting and there's no such check:

  vi->channels=channels;

I can move the check over there if that's the better place.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Nov 8, 2017

Ping?

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Nov 15, 2017

The current code has a bug where if channels <=0 or >= 256 the code would call .oggpack_writeclear on a not yet initialized pack. If added a commit to fix this too.

@agx agx force-pushed the agx:CVE-2017-14633 branch from 7ea509b to 965da65 Nov 20, 2017
…itialized

If the number of channels is not within the allowed range
we call oggback_writeclear altough it's not initialized yet.

This fixes

    =23371== Invalid free() / delete / delete[] / realloc()
    ==23371==    at 0x4C2CE1B: free (vg_replace_malloc.c:530)
    ==23371==    by 0x829CA31: oggpack_writeclear (in /usr/lib/x86_64-linux-gnu/libogg.so.0.8.2)
    ==23371==    by 0x84B96EE: vorbis_analysis_headerout (info.c:652)
    ==23371==    by 0x9FBCBCC: ??? (in /usr/lib/x86_64-linux-gnu/sox/libsox_fmt_vorbis.so)
    ==23371==    by 0x4E524F1: ??? (in /usr/lib/x86_64-linux-gnu/libsox.so.2.0.1)
    ==23371==    by 0x4E52CCA: sox_open_write (in /usr/lib/x86_64-linux-gnu/libsox.so.2.0.1)
    ==23371==    by 0x10D82A: open_output_file (sox.c:1556)
    ==23371==    by 0x10D82A: process (sox.c:1753)
    ==23371==    by 0x10D82A: main (sox.c:3012)
    ==23371==  Address 0x68768c8 is 488 bytes inside a block of size 880 alloc'd
    ==23371==    at 0x4C2BB1F: malloc (vg_replace_malloc.c:298)
    ==23371==    by 0x4C2DE9F: realloc (vg_replace_malloc.c:785)
    ==23371==    by 0x4E545C2: lsx_realloc (in /usr/lib/x86_64-linux-gnu/libsox.so.2.0.1)
    ==23371==    by 0x9FBC9A0: ??? (in /usr/lib/x86_64-linux-gnu/sox/libsox_fmt_vorbis.so)
    ==23371==    by 0x4E524F1: ??? (in /usr/lib/x86_64-linux-gnu/libsox.so.2.0.1)
    ==23371==    by 0x4E52CCA: sox_open_write (in /usr/lib/x86_64-linux-gnu/libsox.so.2.0.1)
    ==23371==    by 0x10D82A: open_output_file (sox.c:1556)
    ==23371==    by 0x10D82A: process (sox.c:1753)
    ==23371==    by 0x10D82A: main (sox.c:3012)

as seen when using the testcase from CVE-2017-11333 with
008d23b applied. However the error was
there before.
@NotTsunami

This comment has been minimized.

Copy link

NotTsunami commented Dec 11, 2017

It would be nice to see this merged and then 1.3.6 shortly after

@tdaede

This comment has been minimized.

Copy link
Contributor

tdaede commented Dec 11, 2017

Rebased and merged as a79ec21 and c1c2831.

A TODO is to move the channel # check earlier, as passing 256 in still won't produce a valid bitstream.

@tdaede tdaede closed this Dec 11, 2017
@mendel129 mendel129 mentioned this pull request Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.