From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29142 invoked by alias); 3 Jul 2019 00:05:22 -0000 Mailing-List: contact bzip2-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: List-Id: Sender: bzip2-devel-owner@sourceware.org Received: (qmail 29067 invoked by uid 89); 3 Jul 2019 00:05:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-19.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=estate, 3003, H*Ad:U*bzip2-devel, stumble X-Spam-Status: No, score=-19.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Message-ID: Subject: Re: Alternative nSelectors patch (Was: bzip2 1.0.7 released) From: Mark Wielaard To: jseward@acm.org, Federico Mena Quintero , bzip2-devel@sourceware.org Date: Tue, 01 Jan 2019 00:00:00 -0000 In-Reply-To: <4f434101-5ce3-d757-2f61-c9e419911e00@acm.org> References: <20190627205837.GD9273@wildebeest.org> <0a2331bc6d0c8500c2c45df1e3ebe01b49ad5831.camel@klomp.org> <8c4d5cf2479253406dacdee122692cc77771afb9.camel@gnome.org> <9998ca428c4c7f895a543aa91941e58efb0d5291.camel@klomp.org> <308d9e82220760205ee673bf0505ee1815d48596.camel@klomp.org> <4f434101-5ce3-d757-2f61-c9e419911e00@acm.org> Content-Type: multipart/mixed; boundary="=-ugTD/7ypNZFRNEMSwZ+R" X-Mailer: Evolution 3.28.5 (3.28.5-2.el7) Mime-Version: 1.0 X-Spam-Flag: NO X-SW-Source: 2019-q3/txt/msg00007.txt.bz2 --=-ugTD/7ypNZFRNEMSwZ+R Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-length: 1874 On Tue, 2019-07-02 at 08:34 +0200, Julian Seward wrote: > This seems to me like a better patch than my proposal, so I retract > my proposal and vote for this one instead. I did keep your cleanup of the compress.c assert. And I don't think I would have proposed something different if I didn't just happen to stumble upon that odd 32767.bz2 testcase. It seems unlikely someone would use so many unused selectors. But the file format allows for it, so lets just handle it. > The one thing that concerned me was that, it would be a disaster -- having > ignored all selectors above 18002 -- if subsequent decoding actually *did* > manage somehow to try to read more than 18002 selectors out of s->selecto= rMtf, > because we'd be reading uninitialised memory. But this seems to me can't > happen because, after the selector-reading loop, you added >=20 > + if (nSelectors > BZ_MAX_SELECTORS) > + nSelectors =3D BZ_MAX_SELECTORS; >=20 > and the following loop: >=20 > /*--- Undo the MTF values for the selectors. ---*/ > ... >=20 > is the only place that reads s->selectorMtf, and then only for the range > 0 .. nSelectors-1. >=20 > So it seems good to me. Does this sync with your analysis? Yes. There is also the other BZ_MAX_SELECTORS sized array s->selector. But that is similarly guarded. First it is filled from the selectorMtf array with that for loop 0 .. nSelectors-1. Then it is accessed through the GET_MTF_VAL macro as selector[groupNo], but that access is guarded with: if (groupNo >=3D nSelectors) \ RETURN(BZ_DATA_ERROR); \ So that prevents any bad access. Attached is the patch with a commit message that hopefully explains why the change is correct (and why the CVE, although a source code bug, wasn't really exploitable in the first place). Hope it makes sense. Cheers, Mark --=-ugTD/7ypNZFRNEMSwZ+R Content-Description: Content-Disposition: inline; filename="0001-Accept-as-many-selectors-as-the-file-format-allows.patch" Content-Type: text/x-patch; name="0001-Accept-as-many-selectors-as-the-file-format-allows.patch"; charset="UTF-8" Content-Transfer-Encoding: base64 Content-length: 4197 RnJvbSBiMzU3ZjRlYzE0YThiNWIxMWIzNzYyMWVlOWYyYTEwZjUxOGI2YzY1 IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJrIFdpZWxhYXJk IDxtYXJrQGtsb21wLm9yZz4KRGF0ZTogV2VkLCAzIEp1bCAyMDE5IDAxOjI4 OjExICswMjAwClN1YmplY3Q6IFtQQVRDSF0gQWNjZXB0IGFzIG1hbnkgc2Vs ZWN0b3JzIGFzIHRoZSBmaWxlIGZvcm1hdCBhbGxvd3MuCgpCdXQgaWdub3Jl IGFueSBsYXJnZXIgdGhhdCB0aGUgdGhlb3JldGljYWwgbWF4aW11bSwgQlpf TUFYX1NFTEVDVE9SUy4KClRoZSB0aGVvcmV0aWNhbCBtYXhpbXVtIG51bWJl ciBvZiBzZWxlY3RvcnMgZGVwZW5kcyBvbiB0aGUgbWF4aW11bQpibG9ja3Np emUgKDkwMDAwMCBieXRlcykgYW5kIHRoZSBudW1iZXIgb2Ygc3ltYm9scyAo NTApIHRoYXQgY2FuIGJlCmVuY29kZWQgd2l0aCBhIGRpZmZlcmVudCBIdWZm bWFuIHRyZWUuIEJaX01BWF9TRUxFQ1RPUlMgaXMgMTgwMDIuCgpCdXQgdGhl IGJ6aXAyIGZpbGUgZm9ybWF0IGFsbG93cyB0aGUgbnVtYmVyIG9mIHNlbGVj dG9ycyB0byBiZSBlbmNvZGVkCndpdGggMTUgYml0cyAoYmVjYXVzZSAxODAw MiBpc24ndCBhIGZhY3RvciBvZiAyIGFuZCBkb2Vzbid0IGZpdCBpbgoxNCBi aXRzKS4gU28gdGhlIGZpbGUgZm9ybWF0IG1heGltdW0gaXMgMzI3Njcgc2Vs ZWN0b3JzLgoKU29tZSBiemlwMiBlbmNvZGVycyBtaWdodCBhY3R1YWxseSBo YXZlIHdyaXR0ZW4gb3V0IG1vcmUgc2VsZWN0b3JzCnRoYW4gdGhlIHRoZW9y ZXRpY2FsIG1heGltdW0gYmVjYXVzZSB0aGV5IHJvdW5kZWQgdXAgdGhlIG51 bWJlciBvZgpzZWxlY3RvcnMgdG8gc29tZSBjb252ZW5pZW50IGZhY3RvciBv ZiA4LgoKVGhlIGV4dHJhIDE0NzY2IHNlbGVjdG9ycyBjYW4gbmV2ZXIgYmUg dmFsaWRseSB1c2VkIGJ5IHRoZSBkZWNvbXByZXNzaW9uCmFsZ29yaXRobS4g U28gd2UgY2FuIHJlYWQgdGhlbSwgYnV0IHRoZW4gZGlzY2FyZCB0aGVtLgoK VGhpcyBpcyBlZmZlY3RpdmVseSB3aGF0IHdhcyBkb25lIChieSBhY2NpZGVu dCkgYmVmb3JlIHdlIGFkZGVkIGEKY2hlY2sgZm9yIG5TZWxlY3RvcnMgdG8g YmUgYXQgbW9zdCBCWl9NQVhfU0VMRUNUT1JTIHRvIG1pdGlnYXRlCkNWRS0y MDE5LTEyOTAwLgoKVGhlIGV4dHJhIHNlbGVjdG9ycyB3ZXJlIHdyaXR0ZW4g b3V0IGFmdGVyIHRoZSBhcnJheSBpbnNpZGUgdGhlCkVTdGF0ZSBzdHJ1Y3Qu IEJ1dCB0aGUgc3RydWN0IGhhcyBleHRyYSBzcGFjZSBhbGxvY2F0ZWQgYWZ0 ZXIgdGhlCnNlbGVjdG9yIGFycmF5cyBvZiAxODA2MCBieXRlcyAod2hpY2gg aXMgbGFyZ2VyIHRoYW4gMTQ3NjYpLgpBbGwgb2Ygd2hpY2ggd2lsbCBiZSBp bml0aWFsaXplZCBsYXRlciAoc28gdGhlIG92ZXJ3cml0ZSBvZiB0aGF0CnNw YWNlIHdpdGggZXh0cmEgc2VsZWN0b3IgdmFsdWVzIHdvdWxkIGhhdmUgYmVl biBoYXJtbGVzcykuCi0tLQogY29tcHJlc3MuYyAgIHwgIDIgKy0KIGRlY29t cHJlc3MuYyB8IDEwICsrKysrKysrLS0KIDIgZmlsZXMgY2hhbmdlZCwgOSBp bnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2Nv bXByZXNzLmMgYi9jb21wcmVzcy5jCmluZGV4IDIzNzYyMGQuLjc2YWRlZTYg MTAwNjQ0Ci0tLSBhL2NvbXByZXNzLmMKKysrIGIvY29tcHJlc3MuYwpAQCAt NDU0LDcgKzQ1NCw3IEBAIHZvaWQgc2VuZE1URlZhbHVlcyAoIEVTdGF0ZSog cyApCiAKICAgIEFzc2VydEgoIG5Hcm91cHMgPCA4LCAzMDAyICk7CiAgICBB c3NlcnRIKCBuU2VsZWN0b3JzIDwgMzI3NjggJiYKLSAgICAgICAgICAgIG5T ZWxlY3RvcnMgPD0gKDIgKyAoOTAwMDAwIC8gQlpfR19TSVpFKSksCisgICAg ICAgICAgICBuU2VsZWN0b3JzIDw9IEJaX01BWF9TRUxFQ1RPUlMsCiAgICAg ICAgICAgICAzMDAzICk7CiAKIApkaWZmIC0tZ2l0IGEvZGVjb21wcmVzcy5j IGIvZGVjb21wcmVzcy5jCmluZGV4IDIwY2U0OTMuLjMzMDM0OTkgMTAwNjQ0 Ci0tLSBhL2RlY29tcHJlc3MuYworKysgYi9kZWNvbXByZXNzLmMKQEAgLTI4 Nyw3ICsyODcsNyBAQCBJbnQzMiBCWjJfZGVjb21wcmVzcyAoIERTdGF0ZSog cyApCiAgICAgICBHRVRfQklUUyhCWl9YX1NFTEVDVE9SXzEsIG5Hcm91cHMs IDMpOwogICAgICAgaWYgKG5Hcm91cHMgPCAyIHx8IG5Hcm91cHMgPiBCWl9O X0dST1VQUykgUkVUVVJOKEJaX0RBVEFfRVJST1IpOwogICAgICAgR0VUX0JJ VFMoQlpfWF9TRUxFQ1RPUl8yLCBuU2VsZWN0b3JzLCAxNSk7Ci0gICAgICBp ZiAoblNlbGVjdG9ycyA8IDEgfHwgblNlbGVjdG9ycyA+IEJaX01BWF9TRUxF Q1RPUlMpIFJFVFVSTihCWl9EQVRBX0VSUk9SKTsKKyAgICAgIGlmIChuU2Vs ZWN0b3JzIDwgMSkgUkVUVVJOKEJaX0RBVEFfRVJST1IpOwogICAgICAgZm9y IChpID0gMDsgaSA8IG5TZWxlY3RvcnM7IGkrKykgewogICAgICAgICAgaiA9 IDA7CiAgICAgICAgICB3aGlsZSAoVHJ1ZSkgewpAQCAtMjk2LDggKzI5Niwx NCBAQCBJbnQzMiBCWjJfZGVjb21wcmVzcyAoIERTdGF0ZSogcyApCiAgICAg ICAgICAgICBqKys7CiAgICAgICAgICAgICBpZiAoaiA+PSBuR3JvdXBzKSBS RVRVUk4oQlpfREFUQV9FUlJPUik7CiAgICAgICAgICB9Ci0gICAgICAgICBz LT5zZWxlY3Rvck10ZltpXSA9IGo7CisgICAgICAgICAvKiBIYXZpbmcgbW9y ZSB0aGFuIEJaX01BWF9TRUxFQ1RPUlMgZG9lc24ndCBtYWtlIG11Y2ggc2Vu c2UKKyAgICAgICAgICAgIHNpbmNlIHRoZXkgd2lsbCBuZXZlciBiZSB1c2Vk LCBidXQgc29tZSBpbXBsZW1lbnRhdGlvbnMgbWlnaHQKKyAgICAgICAgICAg ICJyb3VuZCB1cCIgdGhlIG51bWJlciBvZiBzZWxlY3RvcnMsIHNvIGp1c3Qg aWdub3JlIHRob3NlLiAqLworICAgICAgICAgaWYgKGkgPCBCWl9NQVhfU0VM RUNUT1JTKQorICAgICAgICAgICBzLT5zZWxlY3Rvck10ZltpXSA9IGo7CiAg ICAgICB9CisgICAgICBpZiAoblNlbGVjdG9ycyA+IEJaX01BWF9TRUxFQ1RP UlMpCisgICAgICAgIG5TZWxlY3RvcnMgPSBCWl9NQVhfU0VMRUNUT1JTOwog CiAgICAgICAvKi0tLSBVbmRvIHRoZSBNVEYgdmFsdWVzIGZvciB0aGUgc2Vs ZWN0b3JzLiAtLS0qLwogICAgICAgewotLSAKMS44LjMuMQoK --=-ugTD/7ypNZFRNEMSwZ+R--