Oob read fixes by Konstanty · Pull Request #66 · Konstanty/libmodplug

Konstanty · 2022-01-28T05:30:47Z

Merge several OOB read fixes discovered from a long fuzzing operation (debrouxl).

needs additional checking of pull request Fix misc. crash and leak bugs found by libfuzzer. #58 from AliceLR to be up to date.

- prevents several OOB reads

- ensures good 'state' of variable

- use single % as it wont execute additional code

- this solution is much cleaner

(This is purely cosmetic)

- previously only events were freed.

- adding 1 byte to array

- avoid need to check twice. - there are many more required elements in an OKT file.

- many checks vs mt2instrument - check wDataLen - it might be large enough to be negative in signed equiv. - len might be zero (or len-4)

- check before memcpy - check before reading ps[x] data

- Another octave of possible values, means no OOB read will occur.

- prevents strange negative numbers, - prevents divide by zero

…r it.

… oob_read_fixes

…bmodplug into AliceLR-fuzz-patch-1-original

debrouxl · 2022-01-28T11:37:59Z

Woo, fixes have been merged :)

When the dust has settled a bit, it's time for a new release.
While creating CVE IDs for every single fix made to the library as part of this PR is both undesirable (some issues only have practical impact with instrumented code, AFAICT) and really cumbersome, considering the sheer number of fixes, several CVE IDs for more severe or more visible vulnerabilities (e.g. OOB writes such as aea13cc , UAF, UMR, divide by zero which is pure DoS but doesn't require instrumentation, nullptr deref which is pure DoS under sane OS unless the offset is large) should be requested nevertheless, so as to prod downstreams into upgrading libmodplug.

AliceLR · 2022-01-28T13:03:08Z

So far I've found a couple of minor redundant checks from combining this and my patch that can be revised. I also found more potential breakage in the Oktalyzer loader I somehow never noticed. Will follow up with a small patch after I test this a little more.

edit: yep, 0.1 seconds of fuzzing with UBSan confirmed this loader has major alignment problems.

Konstanty added 30 commits May 29, 2017 06:49

ABC: transpose only needs to look at notes (<26)

f3c1b47

Check memory position isn't over the memory length

d7890bd

- prevents several OOB reads

Initialize nPatterns to 0 earlier

04b7eaf

- ensures good 'state' of variable

ABC: Ensure for loop does not increment past end of loop

f0c1ff2

ABC: Use blankline more often

d387dfd

- use single % as it wont execute additional code

ABC: abort early if macro would be blank

ca73f52

ABC: avoid possibility of incrementing *p

22b2b3c

ABC: clean up loop exiting code

b4867d7

- this solution is much cleaner

ABC: ensure array access is bounded correctly.

0b0d113

ABC: initialize earlier

675ab5d

(This is purely cosmetic)

OKT: ensure file size is enough to contain data

eabcd64

WAV: check that there is space for both headers

23ded13

ABC: cleanup tracks correctly.

9f7821a

- previously only events were freed.

PSM: make sure reads occur of only valid ins

347eeb8

ABC: 10 digit ints require null termination

1162fd5

- adding 1 byte to array

FAR: out by one on check

d0adf30

OKT: add one more bound check

5163a38

ABC: terminate early when things don't work in substitute

f82b63a

ABC: ensure read pointer is valid before incrementing

cdc0842

ABC: prevent possible increment of p past end

4d0bc15

PSM: add missing line to commit

f6dd59a

OKT: increase bound check at start to 12.

1db0c41

- avoid need to check twice. - there are many more required elements in an OKT file.

MED: Ensure that there is room to read one double

083e662

MDL: make sure there is enough space to read the UINT

5523dd6

XM: OOB fixes- don't read items outside 'packsize'

5f4ef8b

MT2: OOB read fixes

1fbf435

- many checks vs mt2instrument - check wDataLen - it might be large enough to be negative in signed equiv. - len might be zero (or len-4)

MDL: OOB read fixes

57a5cf1

- check before memcpy - check before reading ps[x] data

ABC: where char is lower case, allow another octave.

2314d9e

- Another octave of possible values, means no OOB read will occur.

STM: make sure patterns are not loaded above the maximum size

43f0fc4

ABC: bound divider to not be below 1.

1cde769

- prevents strange negative numbers, - prevents divide by zero

AliceLR and others added 22 commits June 19, 2021 04:06

Fix another Extreme's AMS crash.

31910e1

Fix AMS slow loads due to large garbage samples.

ecf8df0

Fix AMS2 instrument bounding checks.

6948105

Add more missing consts in AMS loaders.

4282f68

Add missing AMSUnpack bounds checks.

5e1443f

Fix AMS sample packed length bounding.

2ff82c3

Revise MIDI debug message fix to still use sprintf instead of snprintf.

19a5370

Add missing DMF sample bounds check.

89b591f

Fix WAV fmt header bounds check, add consts to WAV loader.

d8ee117

Shrink AMS samples if the packed size can't possibly fit them.

cd6baf2

Fix hang in WAV loader caused by missing chunk length check.

38a7b2f

Fix XM slow loads caused by bad instrument checks, fix other checks.

f2c6827

Revise FAR max pattern length bounding to be more readable.

6dfcfed

Fix off-by-one AMS pattern bounds checks.

70d73b9

Add extra AMS2 pitchenv bounds check in case support is ever added fo…

f768b60

…r it.

Add missing DMF track bounds check, tweak for consistency.

4317cb7

Merge branch 'master' into oob_read_fixes

5306ac8

Merge branch 'master' into oob_read_fixes

1e38a97

Merge branch 'oob_read_fixes' of github.com:Konstanty/libmodplug into…

6c5cb42

… oob_read_fixes

Merge branch 'fuzz-patch-1-original' of https://github.com/AliceLR/li…

1eec55f

…bmodplug into AliceLR-fuzz-patch-1-original

Merge branch 'AliceLR-fuzz-patch-1-original' into oob_read_fixes

7c1c3aa

Increment version 0.8.9.1

d04fbc2

Konstanty merged commit bfe501c into master Jan 28, 2022

AliceLR mentioned this pull request Jan 28, 2022

Fix misc. crash and leak bugs found by libfuzzer. #58

Closed

AliceLR mentioned this pull request Jan 28, 2022

0.8.9.0: compile time warnings #63

Open

This was referenced Jan 28, 2022

Clean up and fix redundant/missing checks in OKT, S3M, ULT loaders. #73

Merged

Fix more issues from the fuzz-patch-1/oob_read_fixes merge. #75

Merged

Segmentation fault in mmreadUBYTE(MMFILE*) #56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oob read fixes#66

Oob read fixes#66
Konstanty merged 99 commits intomasterfrom
oob_read_fixes

Konstanty commented Jan 28, 2022 •

edited

Loading

Uh oh!

debrouxl commented Jan 28, 2022 •

edited

Loading

Uh oh!

AliceLR commented Jan 28, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Konstanty commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debrouxl commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliceLR commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Konstanty commented Jan 28, 2022 •

edited

Loading

debrouxl commented Jan 28, 2022 •

edited

Loading

AliceLR commented Jan 28, 2022 •

edited

Loading