search

varnish / varnish-modules

Collection of Varnish Cache modules (vmods) by Varnish Software
Other
182 stars 86 forks source link

Revert "Construct intermediate strands on the stack" commit #180

Closed rezan closed 3 years ago

rezan commented 3 years ago

This reverts 81231af3295e1464574cda4dfac238b548e08c19. I believe its not accepted practice to do dynamic stack allocations like this. In this case, there are no bounds checks and also, as with all stack allocations, no way to detect a stack overflow or failure (aside from getting a segfault).

nigoroll commented 3 years ago

As long as we do not have stack limit checks, there is nothing to gain from wasting workspace. I guess there are a lot of arguments to be made here, but the major points might be:

We could actually detect overflows, and I have long tried to advocate for something along these lines, but the argument has been that it was hard to ensure portability. This is a bit unfortunate because I am not aware of any platform at all which used a discontiguous stack.

At any rate, V-S owns this vmod, so you can do whatever you want. But IMHO, for all practical purposes, this change makes no sense at all. If anything, we should start with stack bounds checking before we make PCRE calls.

rezan commented 3 years ago

This means that the strands to be passed would need >4094 elements to have a good chance of overflowing the stack

Right, so we either need to bounds check the strands and limit it to a number of elements or move the allocation off the stack. The challenge here is what is a safe amount and that is very hard or impossible to calculate. This is why dynamic stack allocation is not a good idea. The heap should be used for dynamic allocations.

The idea that dynamic stack allocations are bad in general is pretty dubious as long as we accept unbound call depths.

Technically unbounded recursive calls are not "accepted". You will eventually run out of stack. This is the risk when you use recursion. This is why PCRE has been so problematic for us, it combines recursion with large stack allocations. It would be nice to get them to change to heap allocations, but I might be over simplifying here.

this change makes no sense at all

Without it varnishd will crash when a large strands is used... Are you saying that we should keep this in place and let varnish crash on large values?

nigoroll commented 3 years ago

Are you saying that we should keep this in place and let varnish crash on large values?

Suggestive questions are not helpful. I think I have been clear about what I suggest.

Also I said for all practical purposes, this change makes no sense at all.

The problem you see does exist. But this particular instance of it is virtually irrelevant. And I have explained why.

rezan commented 3 years ago

A little bit lost, but it seems like you are in agreement this needs to be reverted?

nigoroll commented 3 years ago

no

rezan commented 3 years ago

In the hope of resolving this, what do you suggest we do?

nigoroll commented 3 years ago

I suggest to close this PR and discuss in V-C again if we should have generic stack bounds checking.

rezan commented 3 years ago

I will leave this open because we do need to get this resolved soon and it sounds like a V-C discussion will take time to get materialized results. But I am interested in seeing how this would look in Varnish. If the results are good, we can revisit this. Sound good?

nigoroll commented 3 years ago

It is complicated. I took the time for what I hope would be a helpful summary. And I still think you "do not need to get this resolved soon" for the reasons explained before. Do you seriously expect anyone to call header.append() with 4094 strands, and not fail at some other place before? (header.copy() would only ever pass a single strand element)

Anyway, I will try to ignore this ticket from now on, it is not my project.

rezan commented 3 years ago

I think this issue should be closed.

I disagree, but I will let this sit for a few weeks. I consider this a bug and nothing else. I will provide a VTC next time around and I strongly believe that if a certain combination of input can make Varnish panic and that can be replicated in a VTC, it should be fixed. We do this all the time, its hardens the product and this is routine for other products and projects.

rezan commented 3 years ago

So I made a quick poc and was able to easily panic varnishd with a stack overflow by passing in input where the strand size is ~1000. However, the VCL_STRANDS type itself is vulnerable to this kind of attack without even using header.append(), so I will make another issue to address that.

Dynamic allocations are best done on the workspace, worst case on the heap. Dynamic stack allocations are slow, unsafe, constrained, and cannot be easily checked, so the revert puts the allocation back on the workspace which is what we want.

I scanned #3554 and did not see anything in there which can be used here.

gquintard commented 3 years ago

Did we have a consensus on this?

-- Guillaume Quintard

On Fri, Apr 23, 2021, 10:57 Reza Naghibi @.***> wrote:

Merged #180 https://github.com/varnish/varnish-modules/pull/180 into master.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/varnish/varnish-modules/pull/180#event-4638185915, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA42AKISVOHCHIFK3KVFDKTTKGYGZANCNFSM4ZKTBCHA .

dridi commented 3 years ago

Did we have a consensus on this?

To be crystal clear, no. And there was no urgency not to wait a few more days to discuss the PoC findings.