search

albertodemichelis / squirrel

Official repository for the programming language Squirrel
http://www.squirrel-lang.org
MIT License
926 stars 161 forks source link

Out-Of-Bounds access vulnerability 2 #294

Closed radl97 closed 1 week ago

radl97 commented 1 month ago

Hi!

I have discovered a security vulnerability in Squirrel that is present and exploitable in certain cases in real-world projects. I want to responsibly disclose this issue, and I have attempted to contact the author(s). This is second attempt to contact the authors.

Please contact me for more details. I have reported the bug to a vendor who used Squirrel in their product, but I have reason to believe thst they have failed to contact the upstream project, this repository. In the specific case of the vendor, they have verified the exploitability in the context of their project.

Is this project actively maintained?

(By the way, an issue from SonarSource with the same title is present. I think it is about a different issue, but might be duplicate. Was that issue ever resolved?)

albertodemichelis commented 1 month ago

yes, it is maintained but it depends on how much time I have on hands. Let me check my mailbox if I accidentally discarded the mail.

albertodemichelis commented 1 month ago

I can only find an email from SonarSource from 2021 that was already addressed. was this sent to my gmail email?

albertodemichelis commented 1 month ago

my hotmail account is still in use, I might have deleted your email by mistake

radl97 commented 1 month ago

I have sent it again to your hotmail. Did it go through?

albertodemichelis commented 1 month ago

It was in the spam folder, but yes it arrived. thank you :) I guess the previous one went to span as well.

albertodemichelis commented 2 weeks ago

fixed

radl97 commented 2 weeks ago

Hi! I am so sorry to get back on this, but after a smaller investigation, I have found that what I have suggested (and how this was fixed) does not always work, due to C specification shennanigans.

An interesting case none-the-less, as a compiler optimization made available by Undefined Behavior allows compiler to optimize out an otherwise useful security check.

https://godbolt.org/z/aKfa9bq64

(I think I messed up the naming of thr variables)

Here is an example where gcc (trunk) with -O2 can optimize out the check introduced. (I've used reassignment instead of sq_max)

The problem is that according to C specification, signed integer overflow is undefined (UB=undefined behavior), and we are basically checking "did UB happen?" instead of "would UB happen, which is incorrect way to handle these.

I should have seen this when suggesting a fix in private...

My fix would be unsigned addition, which is always defined behavior:

SQInteger newsize = (SQInteger)((SQUnsignedInteger)size + (size>>1);

On the other case, the check could overflow, bht luckily compilers do not seem to optimize the checks. However, avoiding UB would be awesome, because UB means that a newer compiler might just be able to optimize out the security check (which is allowed behavior).

I think these two checks are similar:

if (_scratchpadsize >= (size << 5)) ...
if ((_scratchpadsize >> 5) >= size) ...

However, the latter cannot overflow (hence cannot trigger undefined behavior), so I would also suggest this change also.