Issues found during tinyformat audit (part of Bitcoin Core audit)

[practicalswift]

Hi all,

First, thanks for creating the tinyformat library. Having an easy-to-use locale independent formatting library available under a permissive license is really nice! :)

Bitcoin Core uses tinyformat for formatting of log messages. While auditing Bitcoin Core I discovered the following issues in tinyformat that I thought were worth reporting upstreams.

All issues have been verified against current master.

Issue 1. The following causes a signed integer overflow and a subsequent allocation of 9 GB of RAM (or an OOM in memory constrained environments):

tfm::format("%.777777700000000$", 1.0);

Issue 2. The following causes a stack overflow:

tfm::format("%987654321000000:", 1);

Issue 3. The following causes a stack overflow:

tfm::format("%1$*1$*", -11111111);

Issue 4. The following causes a NULL pointer dereference:

tfm::format("%.1s", (char *)nullptr);

Issue 5. The following causes a float cast overflow:

tfm::format("%c", -1000.0);

Issue 6. The following causes a float cast overflow followed by an invalid integer negation:

tfm::format("%*", std::numeric_limits<double>::lowest());

Note that I've only audited tfm::format(…, …) which is the part of tinyformat used by Bitcoin Core.

Thanks for a nice library!

[c42f]

Hi, thanks for this review! I'd certainly like to fix any of these which reasonably can be fixed or mitigated. Can you describe the types of programming errors or attacks that Bitcoin Core is trying to mitigate against?

I ask because I feel like there's a few classes of possible error and they are not equivalent:

1. Programmer error: Something invalid is passed to tfm::format. For example, issue 4 where a null pointer is passed.
2. User attack: The user is able to influence the data passed to tfm::format 2.1: Users are allowed set the args passed to tfm::format(fmtspec, args...) 2.2: Users are allowed to set fmtspec in tfm::format(fmtspec, args...).

Mitigating programmer error is only ever going to be a best-effort, for example tfm::format("%s", (char *)1) will segfault but this invalid usage cannot be detected by tfm::format. I'm happy to mitigate issue 4 (null pointer dereference) because it's both common and detectable but in general being printf compatible means accepting bare pointers which we cannot validate.

I feel tinyformat should defend against user attacks of type 2.1. Cases where the user can crash the program by providing out-of-domain args for a fixed, valid fmtspec seems like a bug.

I'm not yet sure it's even possible to defend effectively against 2.2 where users have access to fmtspec. For example, tfm::format(fmtspec, (char*)1) is valid when fmtspec="%p" but a segfault when fmtspec="%s". I've always thought that giving the user access to fmtspec is fundamentally unsafe and a bad idea (it certainly is with C's printf). It may be possible to make safe in tinyformat with some work but I currently don't recommend it. What are Bitcoin Core's expectations about this?

[practicalswift]

@c42f

I cannot speak for Bitcoin Core, but for me personally the reason I like tinyformat is that it is more robust against malformed input compared to the alternatives.

If we can make it more robust I'll like it even more :)

I really like robustness. Ideally (and if possible) both in terms of robustness against programmer error and in terms of robustness against user attack. In some cases this ideal state is not possible: as you note the invalid pointer case cannot be guarded against.

AFAICT the maximally robust version for tfm::format would have a contract along the lines of:

Assuming that arg in tfm::format(fmtspec, arg) is not an invalid pointer
then the function is guaranteed to a.) return a formatted std::string, or
b.) TINYFORMAT_ERROR(…) will be called.

As a user of the library I would love such a contract as it is maximally robust against crazy input (regardless of the reason for said crazy input) :)

Besides the invalid pointer case, can we think of any failure scenario that is technically impossible to guard against?

[maflcko]

In Bitcoin Core, all format strings are hardcoded and I don't think we pass in pointers in the args, so our usage should be fine.

[practicalswift]

@MarcoFalke Under the assumption of no programmer mistakes, yes. It would be strictly better if we had robustness also in the presence of programmer mistakes (which is entirely possible with the notable exception of invalid pointers mentioned above), no? Robustness is an undervalued property :)

[practicalswift]

Some additional minor issues found:

strprintf("%c", 1.31783e+38);
tinyformat.h:244:36: runtime error: 1.31783e+38 is outside the range of representable values of type 'char'
strprintf("%c", 2.54176e+307);
tinyformat.h:244:36: runtime error: 2.54176e+307 is outside the range of representable values of type 'char'

strprintf("%*", -2.33527e+38);
tinyformat.h:283:65: runtime error: -2.33527e+38 is outside the range of representable values of type 'int'
strprintf("%*", -1.45258e+308);
tinyformat.h:283:65: runtime error: -1.45258e+308 is outside the range of representable values of type 'int'
strprintf("%*", -2147483648);
tinyformat.h:763:25: runtime error: negation of -2147483648 cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself