Add advisory for bigint

[elichai]

Hi,
The bigint library(https://github.com/paritytech/bigint) is a copy of very old code(written pre-1.0 in 2014) and contains a bunch of UB, including uninitialized memory, pointers out of bounds etc.

Even the simplest example triggers UB in miri: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=5f059a27e245cad9f9e49be2b68678c8
(Sadly it seems that there's a bunch of cryptographic libraries depending on this https://crates.io/crates/bigint/reverse_dependencies )

[WaffleLapkin]

The bigint's readme even states that

This crate is deprecated and will not be developed further. Users are invited to prefer the uint crate instead.

I think we have to

1. mention that the crate is also unmaintained
2. propose alternative crate(s) (e.g. uint)

[elichai]

2. propose alternative crate(s) (e.g. [uint](https://crates.io/crates/uint))

I'd not comfortably propose uint as looking at that code it seems they kept a lot of the old code but patched the UB they found https://github.com/paritytech/parity-common/blob/master/uint/src/uint.rs.
So I personally can't propose it without checking it myself, which I'm not sure I'll have the time. (especially knowing how hard is it to implement something like that without having a carry bug)
Anyway I still think we should put out the advisory so we can warn users of this crate about it.
Should I change the advisory to deprecated?

[tarcieri]

I think it's probably worth filing two advisories for bigint: a security vulnerability for the UB, and an informational advisory that it's unmaintained.

[elichai]

I think it's probably worth filing two advisories for bigint: a security vulnerability for the UB, and an informational advisory that it's unmaintained.

So if you're ok with that, let's leave this as the vulnerability and someone who feels comfortable recommending a different bignum library will create the unmaintained one?
(if we don't have to recommend anything I don't mind adding here the unmaintained one)

[tarcieri]

Sounds good

[fogti]

I think the formatting of the UB vuln advisory should be fixed. It contains double newlines and a newline as first line...

[elichai]

I think the formatting of the UB vuln advisory should be fixed. It contains double newlines and a newline as first line...

Will fix, mostly because I had to delete all the comments from the sample :)

[tarcieri]

These sentences should end in periods.

It'd also probably be good to be a little more specific about what the problems are.

[ordian]

Hi there, maintainer of uint here.

If this vulnerability is the same as paritytech/parity-common#380, I want to point out that it's not possible to exploit it in any way, since the misconstructed pointers are never dereferenced. In fact it's typical for C++ code to create such pointers and it's not considered UB there

// v.end() is out of bounds
for (std::vector<int>::iterator it = v.begin() ; it != v.end(); ++it) {...}

(Sadly it seems that there's a bunch of cryptographic libraries depending on this https://crates.io/crates/bigint/reverse_dependencies )

The bigint crate was deprecated in favor of uint 2 years ago as you can see from its readme. It seems to be used in a couple of hobby zk libs, I wouldn't call it "a bunch of cryptographic libraries".

[tarcieri]

@ordian any advice for the types of advisories we should file here? Do you think a vulnerability advisory is warranted, or should we just file an informational advisory that bigint is unmaintained pointing at uint (as suggested earlier)?

[ordian]

@ordian any advice for the types of advisories we should file here? Do you think a vulnerability advisory is warranted, or should we just file an informational advisory that bigint is unmaintained pointing at uint (as suggested earlier)?

Thanks for the quick reply! I think an informational advisory suggesting the use of uint should be enough. I don't any reason why new crates would consider using bigint seeing it being deprecated in favor of uint.

[elichai]

In fact it's typical for C++ code to create such pointers and it's not considered UB there

FWIW, C and C++ allow pointers to point to one past the last element of the array/object(but not dereference that pointer) but rust gives you 2 options, offset which isn't allowed to do that and wraping_offset which is allowed.

[RalfJung]

@elichai we now have an "unsound" kind of informational advisories; maybe it would make sense to update this PR to use that (see #316 for an example)?

[RalfJung]

FWIW, C and C++ allow pointers to point to one past the last element of the array/object(but not dereference that pointer) but rust gives you 2 options, offset which isn't allowed to do that and wraping_offset which is allowed.

Btw this is not correct; offset in Rust allows one-past-the-end pointers just like C/C++ pointer arithmetic.

[RalfJung]

The problem here was that the code created a pointer to one before the allocated object (fixed in paritytech/parity-common#381). This is UB in C/C++ as well as in Rust.

@ordian

In fact it's typical for C++ code to create such pointers and it's not considered UB there

So this is just not correct I think -- the same code is UB in C++ as well.

[ordian]

@RalfJung https://en.cppreference.com/w/cpp/container/vector/rend, I don't know how it's implemented in practice though.

EDIT: Ok, I think you're right. Anyway, this PR is for bigint which is deprecated and probably has other bugs.

[RalfJung]

The C++ standard says

If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.

So I maintain my claim that having a pointer to one before an allocation is UB in C/C++. The reverse iterator probably has to do something clever to avoid that UB.

[RalfJung]

Anyway, this PR is for bigint which is deprecated and probably has other bugs.

Good point; I opened #338 to reflect the maintenance state.

[elichai]

Sorry I wasn't really following, should I close this? should I change something?

Btw this is not correct; offset in Rust allows one-past-the-end pointers just like C/C++ pointer arithmetic.

Ha! I see that the docs also say that, that's weird I remember explicitly that this was another instance of difference between rust and C, but I guess I remembered wrong, ha.

[RalfJung]

@elichai you have some comments requesting changes, but IMO what makes more sense is to close this PR and issue a deprecation advisory instead (which is what #338 does). YMMV though.

[elichai]

yeah I think #338 has more chance of getting merged(it's been 2 months here), and unlike me @RalfJung is comfortable recommending another crate, which is probably more helpful.

[RalfJung]

I am not recommending anything, I am just quoting the bigint crate page.