Skip to content

A flagged search tries one start offset too many #3558

Closed

Description

@ESultanik

Summary

A search/N that carries any string flag tries one start offset too many. libmagic's last candidate
is N - 1; PolyFile allows N. With no flag set the two agree, because libmagic then takes a
different code path whose window happens to match what PolyFile computes.

Reproducer

ABC sits at offset 4, and search/4 allows offsets 0 through 3:

$ for d in search/4 search/4/c search/4/w search/4/W; do \
    printf '0\t%s\tABC\tfound\n' "$d" > reps.magic; \
    printf 'xxxxABC' > in.bin; \
    printf '%-12s -> %s\n' "$d" "$(TZ=UTC ./file/src/file -b -m reps.magic in.bin)"; done
search/4     -> found, ASCII text, with no line terminators
search/4/c   -> ASCII text, with no line terminators
search/4/w   -> ASCII text, with no line terminators
search/4/W   -> ASCII text, with no line terminators

PolyFile on master at ed571ba reports found for all four.

Cause

magiccheck has two implementations of the search, and they disagree by one offset.

The loop runs idx over [0, m->str_range), so the last start offset it tries is str_range - 1
(file/src/softmagic.c:2355-2368).

The memmem fast path ahead of it runs only when m->str_flags == 0, and it searches a window of
m->str_range + slen bytes (file/src/softmagic.c:2334-2353). A value of slen bytes can therefore
start as late as offset str_range in that window. So a flagless search reaches one offset further
than a flagged one.

StringMatch.search computes end_pos = min(len(data), self.num_bytes + len(self.string)), which is
the memmem window. That is right for a flagless search and one offset too generous for every other
one.

Impact

A false positive: a search/N with a flag reports a value that begins exactly N bytes into the
buffer, where file reports nothing. No corpus stem covers it. The magnitude is one offset, so a
definition that searches a wide window is unlikely to be affected in practice.

Note that memmem is conditional on HAVE_MEMMEM; a build of file without it would answer the
flagless case the same way as the flagged one. The reference build used above has it.

Found while fixing #3552.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions