[Bug 16255] New: Static code analyses show issues in parsing data safety

[bugzilla at busybox.net]

https://bugs.busybox.net/show_bug.cgi?id=16255

            Bug ID: 16255
           Summary: Static code analyses show issues in parsing data
                    safety
           Product: Busybox
           Version: unspecified
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: Other
          Assignee: unassigned at busybox.net
          Reporter: marcin.w.nowakowski at gmail.com
                CC: busybox-cvs at busybox.net
  Target Milestone: ---

In function check_header_gzip (file: decompress_gunzip.c), there is direct
usage of a received offset of extended header information, without any check if
the offset goes beyond of the allocated buffer. Of-course, to trigger this
error there would be required to prepare an intentionally wrongly compressed
gzip file, with wrong header offsets.
However, such artificially prepared gzip files could cause real issues in any
working system.
Any fix is not so easy as it requires introducing additional checks of the
extended fields, which for now is ignored.

I have two questions:
1. Such errors from static code analyzers - would they be a subject for fixes?
2. Or the basic idea of keeping busybox as small as possible is top priority
and there is no sense to provide fixes of such type?

The full code analyses report:
static int check_header_gzip(STATE_PARAM transformer_state_t *xstate)
1141{
1142        union {
1143                unsigned char raw[8];
1144                struct {
1145                        uint8_t gz_method;
1146                        uint8_t flags;
1147                        uint32_t mtime;
1148                        uint8_t xtra_flags_UNUSED;
1149                        uint8_t os_flags_UNUSED;
1150                } PACKED formatted;
1151        } header;
1152
1153        BUILD_BUG_ON(sizeof(header) != 8);
1154
1155        /*
1156         * Rewind bytebuffer. We use the beginning because the header has 8
1157         * bytes, leaving enough for unwinding afterwards.
1158         */
1159        bytebuffer_size -= bytebuffer_offset;
1160        memmove(bytebuffer, &bytebuffer[bytebuffer_offset],
bytebuffer_size);
1161        bytebuffer_offset = 0;
1162
1. Condition !top_up(state, 8), taking false branch.
1163        if (!top_up(PASS_STATE 8))
1164                return 0;
1165        memcpy(header.raw, &bytebuffer[bytebuffer_offset], 8);
1166        bytebuffer_offset += 8;
1167
1168        /* Check the compression method */
2. Condition header.formatted.gz_method != 8, taking false branch.
1169        if (header.formatted.gz_method != 8) {
1170                return 0;
1171        }
1172
3. Condition header.formatted.flags & 4, taking true branch.
1173        if (header.formatted.flags & 0x04) {
1174                /* bit 2 set: extra field present */
1175                unsigned extra_short;
1176
4. tainted_argument: Calling function top_up taints argument
*state->bytebuffer.["show details"]
5. Condition !top_up(state, 2), taking false branch.
1177                if (!top_up(PASS_STATE 2))
1178                        return 0;
6. tainted_data_transitive: Call to function buffer_read_le_u16 with tainted
argument *state->bytebuffer returns tainted data.["show details"]
7. var_assign: Assigning: extra_short = buffer_read_le_u16(state), which taints
extra_short.
1179                extra_short = buffer_read_le_u16(PASS_STATE_ONLY);
8. Condition !top_up(state, extra_short), taking false branch.
1180                if (!top_up(PASS_STATE extra_short))
1181                        return 0;
1182                /* Ignore extra field */
9. var_assign_var: Compound assignment involving tainted variable extra_short
to variable state->bytebuffer_offset taints state->bytebuffer_offset.
1183                bytebuffer_offset += extra_short;
1184        }
1185
1186        /* Discard original name and file comment if any */
1187        /* bit 3 set: original file name present */
1188        /* bit 4 set: file comment present */
10. Condition header.formatted.flags & 0x18, taking true branch.
1189        if (header.formatted.flags & 0x18) {
1190                while (1) {
1191                        do {
CID 5896545: (#3 of 3): Untrusted value as argument (TAINTED_SCALAR)
11. tainted_data: Passing tainted expression state->bytebuffer_offset to
top_up, which uses it as an offset.["show details"]
Ensure that tainted values are properly sanitized, by checking that their
values are within a permissible range.
1192                                if (!top_up(PASS_STATE 1))
1193                                        return 0;
CID 5896545:(#2 of 3):Untrusted value as argument (TAINTED_SCALAR) [ "select
issue" ]
1194                        } while (bytebuffer[bytebuffer_offset++] != 0);
1195                        if ((header.formatted.flags & 0x18) != 0x18)
1196                                break;
1197                        header.formatted.flags &= ~0x18;
1198                }
1199        }

-- 
You are receiving this mail because:
You are on the CC list for the bug.