Re: [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling

[Ravi Mistry <rmistry@google.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Maybe I am slow, but I do not immediately see why ignoring
> everything after the first NUL is a problem.  A call to
> strbuf_trim() is ineffective at trimming whitespace that appears
> immediately before such a NUL.  For example, while
>
>     cf9bdb1...f6092d # comment LF
>
> would feed the leading 'cf9bdb1...f6092d' part (after stripping
> whitespace before '#') to parse_oid_hex_algop(), this
>
>     cf9bdb1...f6092d NUL comment LF
>
> would keep the whitespace after '92d' and cause the parsing to
> fail.  I do not see any security implications here.

> > +		if (memchr(sb.buf, '\0', sb.len))
> > +			die("invalid object name: %s", sb.buf);
>
> A file with such an entry is rejected and the entire operation is
> aborted as suspected attack attempt, which feels like striking the
> balance between usability and security at a wrong place.
>
> But a line with broken object name already is rejected with "die()"
> with the existing code, so it may be OK.

Agreed, there is no security implication there. Rejecting
"<oid>\0garbage" (when there is no space before the NUL) was only for
consistency with how other non-comment trailing garbage on a line is
rejected with die(). If we end up rerolling the series, I am happy to
drop the NUL check if you prefer.

Thanks!