Hi Moemen,
any chance to get this feature before 2.4 will be realeased?
Regards
Aleks
On 06.04.21 09:13, Willy Tarreau wrote:
Hi Moemen,
On Tue, Apr 06, 2021 at 01:58:11AM +0200, Moemen MHEDHBI wrote:
Only part unclear:
On 02/04/2021 15:04, Tim Düsterhus wrote:
+int base64urldec(const char *in, size_t ilen, char *out, size_t olen) {
+ char conv[ilen+2];
This looks like a remotely triggerable stack overflow.
You mean in case ilen is too big?
Yes that's it, I didn't notice it during the first review. It's
particularly uncommon to use variable sized arrays and should never
be done. The immediate effect of this is that it will reserve some
room in the stack for a size as large as ilen+2 bytes. The problem
is that on most platforms the stack grows down, so the beginning of
the buffer is located at a point which is very far away from the
current stack. This memory is in fact not allocated, so the system
detects the first usage through a page fault and allocates the
necessary space. But in order to know that the page fault is within
the stack, it has to apply a certain margin. And this margin varies
between OSes and platforms. Some compilers will explicitly initialize
such a large stack from top to bottom to avoid a crash. Other ones
will not do and may very well crash at 64kB. On Linux, I can make the
above crash by using a 8 MB ilen, just because by default the stack
size limit is 8 MB. That's large but not overly excessive for those
who would like to perform some processing on bodies. And I recall
that some other OSes default to way smaller limits (I recall 64kB
on OpenBSD a long time ago though this might have been raised to a
megabyte or so by now).
in such case should we rather use dynamic allocation ?
No, there are two possible approaches. One of them is to use a trash
buffer using get_trash_chunk(). The trash buffers are "large enough"
for anything that comes from outside. A second, cleaner solution
simply consists in not using a temporary buffer but doing the conversion
on the fly. Indeed, looking closer, what the function does is to first
replace a few chars on the whole chain to then call the base64 conversion
function. So it doubles the work on the string and one side effect of
this double work is that you need a temporary storage.
Other approaches would consist in either reimplementing the functions
with a different alphabet, or modifying the existing ones to take an
extra argument for the conversion table, and make one set of functions
making use of the current table and another set making use of your new
table.
Willy
