Re: Is MinMaxExpr really leakproof?

Greetings,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> So I'd like to get to a point where we're comfortable marking these
> >> functions leakproof despite the possibility of corner-case failures.
> >> We could just decide that the existing failure cases in varstr_cmp are
> >> not usefully exploitable for information leakage, or perhaps we could
> >> dumb down the error messages some more to make them even less so.
> >> It'd also be nice to have some articulatable policy that encompasses
> >> a choice like this.
>
> > I'd rather not say "well, these are mostly leakproof and therefore it's
> > good enough" unless those corner-case failures you're referring to are
> > really "this system call isn't documented to ever fail in a way we can't
> > handle, but somehow it did and we're blowing up because of it."
>
> Well, that's pretty much what we've got here.

Good. Those all almost certainly fall under the category of 'covert
channels' and provided they're low bandwidth and hard to control, as
seems to be the case here, then I believe we can accept them. I'm
afraid there isn't really any hard-and-fast definition that could be
used as a basis for a project policy around this, unfortunately. We
certainly shouldn't be returning direct data from the heap or indexes as
part of error messages in leakproof functions, and we should do our best
to ensure that anything from system calls we make also don't, but
strerror-like results or the error codes themselves should be fine.

> 1. As Noah noted, every failure case in varstr_cmp is ideally a can't
> happen case, since if it could happen on valid data then that data
> couldn't be put into a btree index.

That's certainly a good point.

> 2. AFAICS, all the error messages in question just report that a system
> operation failed, with some errno string or equivalent; none of the
> original data is reported. (Obviously, we'd want to add comments
> discouraging people from changing that ...)

Agreed, we should definitely add comments here (and, really, in any
other cases where we need to be thinking about similar issues..).

> Conceivably, an attacker could learn the length of some long string
> by noting a palloc failure report --- but we've already accepted an
> equivalent hazard in texteq or byteaeq, I believe, and anyway it's
> pretty hard to believe that an attacker could control such failures
> well enough to weaponize it.

Right, that's a low bandwidth covert channel and as such should be
acceptable.

> So the question boils down to whether you think that somebody could
> infer something else useful about the contents of a string from
> the strerror (or ICU u_errorName()) summary of a system function
> failure. This seems like a pretty thin argument to begin with,
> and then the presumed difficulty of making such a failure happen
> repeatably makes it even harder to credit as a useful information
> leak.
>
> So I'm personally satisfied that we could mark text_cmp et al as
> leakproof, but I'm not sure how we define a project policy that
> supports such a determination.

I'm not sure how to formalize such a policy either though perhaps we
could discuss specific "don't do this" things and have a light
dicussion about what covert channel are.

Thanks!

Stephen