Skip to content

Commit 126b2a4

Browse files
committed
RAR3 unrar code: Tweaks
Add more sanity checks/reject logic, that also helps robustness by avoiding out of bounds reads. Also adds a test asserting that the inflated size meets the expections. See openwall#5653.
1 parent 2737437 commit 126b2a4

File tree

1 file changed

+24
-3
lines changed

1 file changed

+24
-3
lines changed

src/unrar.c

+24-3
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*
22
* Extract RAR archives
33
*
4-
* Modified for JtR, (c) magnum 2012. This code use a memory buffer instead
4+
* Modified for JtR, (c) magnum 2012-2025. This code uses a buffer instead
55
* of a file handle, and decrypts while reading. It does not store inflated
66
* data, it just CRC's it. Support for older RAR versions was stripped.
77
* Autoconf stuff was removed.
@@ -31,6 +31,8 @@
3131
#include <stdlib.h>
3232
#include <string.h>
3333

34+
#include "common.h"
35+
3436
#include "unrar.h"
3537
#include "unrarppm.h"
3638
#include "unrarvm.h"
@@ -1054,6 +1056,7 @@ int rar_unpack29(const unsigned char *fd, int solid, unpack_data_t *unpack_data)
10541056
} else {
10551057
number = rar_decode_number(unpack_data, (struct Decode *)&unpack_data->LD);
10561058
//rar_dbgmsg("number = %d\n", number);
1059+
/* Sanity check added by magnum */
10571060
if (number < 0) {
10581061
retval = 0;
10591062
break;
@@ -1063,14 +1066,20 @@ int rar_unpack29(const unsigned char *fd, int solid, unpack_data_t *unpack_data)
10631066
continue;
10641067
}
10651068
if (number >= 271) {
1069+
/* Sanity check added by magnum (lbits is same size) */
1070+
if (number - 271 >= sizeof(ldecode)) {
1071+
retval = 0;
1072+
break;
1073+
}
10661074
length = ldecode[number-=271]+3;
10671075
if ((bits=lbits[number]) > 0) {
10681076
length += rar_getbits(unpack_data) >> (16-bits);
10691077
rar_addbits(unpack_data, bits);
10701078
}
10711079
dist_number = rar_decode_number(unpack_data,
10721080
(struct Decode *)&unpack_data->DD);
1073-
if (dist_number < 0) {
1081+
/* Sanity checks added by magnum (dbits is same size) */
1082+
if (dist_number < 0 || dist_number >= sizeof(ddecode)) {
10741083
retval = 0;
10751084
break;
10761085
}
@@ -1149,7 +1158,8 @@ int rar_unpack29(const unsigned char *fd, int solid, unpack_data_t *unpack_data)
11491158

11501159
length_number = rar_decode_number(unpack_data,
11511160
(struct Decode *)&unpack_data->RD);
1152-
if (length_number < 0) {
1161+
/* Sanity checks added by magnum (lbits is same size) */
1162+
if (length_number < 0 || length_number >= sizeof(ldecode)) {
11531163
retval = 0;
11541164
break;
11551165
}
@@ -1163,6 +1173,11 @@ int rar_unpack29(const unsigned char *fd, int solid, unpack_data_t *unpack_data)
11631173
continue;
11641174
}
11651175
if (number < 272) {
1176+
/* Sanity check added by magnum (sdbits is same size) */
1177+
if (number - 263 >= sizeof(sddecode)) {
1178+
retval = 0;
1179+
break;
1180+
}
11661181
distance = sddecode[number-=263]+1;
11671182
if ((bits = sdbits[number]) > 0) {
11681183
distance += rar_getbits(unpack_data) >> (16-bits);
@@ -1180,6 +1195,12 @@ int rar_unpack29(const unsigned char *fd, int solid, unpack_data_t *unpack_data)
11801195
unp_write_buf(unpack_data);
11811196
}
11821197

1198+
/* Added by magnum */
1199+
if (retval && unpack_data->written_size != unpack_data->dest_unp_size) {
1200+
//rar_dbgmsg("Passed but only wrote %ld of %ld, degrading to FAIL\n", (long)unpack_data->written_size, (long)unpack_data->dest_unp_size);
1201+
retval = 0;
1202+
}
1203+
11831204
//rar_dbgmsg("Written size: %ld\n", (long)unpack_data->written_size);
11841205
//rar_dbgmsg("True size: %ld\n", (long)unpack_data->true_size);
11851206

0 commit comments

Comments
 (0)