Re: Is MinMaxExpr really leakproof?

Noah Misch <noah@leadboat.com> writes:
> This thread duplicates https://postgr.es/m/flat/16539.1431472961%40sss.pgh.pa.us

Ah, so it does.  Not sure why that fell off the radar without getting
fixed; possibly because it was right before PGCon.

> pg_lsn_cmp() and btoidvectorcmp() surely could advertise leakproofness.

Agreed; I'll go fix those.

> I'm not sure about enum_cmp(), numeric_cmp(), tsquery_cmp() or
> tsvector_cmp().  I can't think of a reason those would leak, though.

I've not looked at the last three, but enum_cmp can potentially report the
value of an input, if it fails to find a matching pg_enum record:

        enum_tup = SearchSysCache1(ENUMOID, ObjectIdGetDatum(arg1));
        if (!HeapTupleIsValid(enum_tup))
            ereport(ERROR,
                    (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
                     errmsg("invalid internal value for enum: %u",
                            arg1)));

and there are similar error reports inside compare_values_of_enum().
Whether that amounts to an interesting security leak is debatable.
It's hard to see how an attacker could arrange for those to fail,
much less do so in a way that would reveal a value he didn't know
already.

> btrecordcmp() and other polymorphic
> cmp functions can fail:
>   create type boxrec as (a box); select '("(1,1),(0,0)")'::boxrec = '("(1,1),(0,0)")'::boxrec;
>   => ERROR:  could not identify an equality operator for type box
> The documentation says, "a function which throws an error message for some
> argument values but not others ... is not leakproof."  I would be comfortable
> amending that to allow the "could not identify an equality operator" error,
> because that error follows from type specifics, not value specifics.

I think the real issue with btrecordcmp, btarraycmp, etc, is that
they invoke other type-specific comparison functions.  Therefore,
we cannot mark them leakproof unless every type-specific comparison
function is leakproof.  So we're right back at the policy question.

> bttextcmp() and other varstr_cmp() callers fall afoul of the same restriction
> with their "could not convert string to UTF-16" errors
> (https://postgr.es/m/CADyhKSXPwrUv%2B9LtqPAQ_gyZTv4hYbr2KwqBxcs6a3Vee1jBLQ%40mail.gmail.com).
> Leaking the binary fact that an unspecified string contains an unspecified rare
> Unicode character is not a serious leak, however.  Also, those errors would be a
> substantial usability impediment if they happened much in practice; you couldn't
> index affected values.

Yeah.  I think that there might be a usability argument for marking
textcmp and related operators as leakproof despite this theoretical
leak condition, because not marking them puts a large practical
constraint on what conditions we can optimize.  However, that
discussion just applies narrowly to the string data types; it is
independent of what we want to say the general policy is.

>> I think that we should either change contain_leaked_vars_walker to
>> explicitly verify leakproofness of the comparison function, or decide
>> that it's project policy that btree comparison functions are leakproof,
>> and change the markings on those (and their associated operators).

> Either of those solutions sounds fine.  Like last time, I'll vote for explicitly
> verifying leakproofness.

Yeah, I'm leaning in that direction as well.  Other than comparisons
involving strings, it's not clear that we'd gain much from insisting
on leakproofness in general, and it seems like it might be rather a
large burden to put on add-on data types.

            regards, tom lane