Bug #2484

Some potential bugs in lighttpd-1.4.32

Added by zhenbxoxu 12 months ago. Updated 11 months ago.

Status:FixedStart date:2013-05-05
Priority:NormalDue date:
Assignee:-% Done:

100%

Category:-
Target version:1.4.33
Missing in 1.5.x:No

Description

Hi,
I'm a developer of a static analysis tool canalyze.
Recently I applied it to lighttpd-1.4.32.
It seems some reports are real after by manually checing:
1. Use Undefined Value
file: mod_redirect.c
function: SETDEFAULTS_FUNC
At line 86: data_array *da = (data_array *)du;
du is undefined value.

2. Use Undefined Value
file: http_auth.c
function: base64_decode
At line 95: buffer_prepare_copy(out, in_len);
out->ptr may be a newly allocated memory object.

At line 109: if (ch < 0) continue;
ch < 0 is satifiable at the first iteration, i.e. i 0

And at the second iteration, i 1
At line 116: result[j++] |= ch >> 4;
result[j] is a undefined value.

3. Memory leak
file: mod_simple_vhost.c
function: build_doc_root
At line 166: Do we need to free the buffer in out?

4. Memory leak
file: proc_open.c
function: proc_open_buffer
At line 312: buffer *tmp = buffer_init();
Create a new buffer here.

At line 320: Do we need to free the buffer when error writing pipe.

5. Memory leak
file: fdevent.c
function: fdevent_init
At line 68: if (0 != fdevent_libev_init(ev))
"ev" is not released when event-handler libev failed.

6. Memory leak
file: lemon.c
function: tplt_open
At line 2867: tpltname = pathsearch(lemp->argv0,lemp->tmplname,0);
Memory is allocatd here but not released when return.

7. Use After free
file: mod_fastcgi.c
function: fastcgi_get_packet
At line 2426: buffer_free(packet->b);
Buffer is freed and is used
At line 2429: log_error_write(srv, FILE, LINE, "sdsds", "FastCGI: header too small:", packet->b->used, "bytes <", sizeof(FCGI_Header), "bytes, waiting for more data");

8. Memory leak
file: network_linux_sendfile.c
function: network_write_chunkqueue_linuxsendfile
At line 180: if (HANDLER_ERROR == stat_cache_get_entry(srv, con, c->file.name, &sce)) {
Memory is allocated to sce,

Should we release "sce" at line 189 and 193?

9. Memory leak
file: lemon.c
function: FindStates
At line 744: (void)getstate(lemp);
would return a heap object

And it also reports hundreds of null pointer dereferences caused by unchecked malloc failure. For example:
file: array.c
function: array_init_array
At line 32: a->data = malloc(...)
At line 34: if (src->data[i]) a->data[i] = src->data[i]->copy(src->data[i]);
use a->data without failure checking.

Hope for your replies! Thank you!

Regards,
Zhenbo Xu

Associated revisions

Revision 2871
Added by stbuehler 11 months ago

[mod_auth] fix base64_decode (#2484)

Revision 2872
Added by stbuehler 11 months ago

fix some bugs found with canalyze (fixes #2484, thx Zhenbo Xu)

History

#1 Updated by stbuehler 12 months ago

  • Target version set to 1.4.33

Hi!

zhenbxoxu wrote:

Hi,
I'm a developer of a static analysis tool canalyze.
Recently I applied it to lighttpd-1.4.32.

Thanks :)

It seems some reports are real after by manually checing:
1. Use Undefined Value
file: mod_redirect.c
function: SETDEFAULTS_FUNC
At line 86: data_array *da = (data_array *)du;
du is undefined value.

Yes... otoh, the assigned undefined value is never actually used.

2. Use Undefined Value
file: http_auth.c
function: base64_decode
At line 95: buffer_prepare_copy(out, in_len);
out->ptr may be a newly allocated memory object.

At line 109: if (ch < 0) continue;
ch < 0 is satifiable at the first iteration, i.e. i 0

And at the second iteration, i 1
At line 116: result[j++] |= ch >> 4;
result[j] is a undefined value.

Oh this looks ugly...
As it skips invalid characters (I think we could just fail on invalid characters) it should keep the "state % 4" in a separate var.
Also the padding seems to insert a 0 character as data (it counts to out->used).

3. Memory leak
file: mod_simple_vhost.c
function: build_doc_root
At line 166: Do we need to free the buffer in out?

I don't see a buffer allocation, why would we free one? The buffer gets reused, no need to empty it every time.

4. Memory leak
file: proc_open.c
function: proc_open_buffer
At line 312: buffer *tmp = buffer_init();
Create a new buffer here.

At line 320: Do we need to free the buffer when error writing pipe.

We could. Otoh, the error in this place will make lighttpd exit anyway (only used for config include_shell).

5. Memory leak
file: fdevent.c
function: fdevent_init
At line 68: if (0 != fdevent_libev_init(ev))
"ev" is not released when event-handler libev failed.

Same as 4 - without an event-handler lighttpd won't start.

6. Memory leak
file: lemon.c
function: tplt_open
At line 2867: tpltname = pathsearch(lemp->argv0,lemp->tmplname,0);
Memory is allocatd here but not released when return.

Just a parser generator, only used for building. There are probably newer versions of lemon.

7. Use After free
file: mod_fastcgi.c
function: fastcgi_get_packet
At line 2426: buffer_free(packet->b);
Buffer is freed and is used
At line 2429: log_error_write(srv, FILE, LINE, "sdsds", "FastCGI: header too small:", packet->b->used, "bytes <", sizeof(FCGI_Header), "bytes, waiting for more data");

Right. Only debug option and FastCGI backends have other ways to cause DoS (OOM trigger by sending large responses to slow clients) => not a security issue.

8. Memory leak
file: network_linux_sendfile.c
function: network_write_chunkqueue_linuxsendfile
At line 180: if (HANDLER_ERROR == stat_cache_get_entry(srv, con, c->file.name, &sce)) {
Memory is allocated to sce,

Should we release "sce" at line 189 and 193?

No. stat cache entries are kept in a tree for later reuse. The mainloop triggers cleaning each second.

9. Memory leak
file: lemon.c
function: FindStates
At line 744: (void)getstate(lemp);
would return a heap object

See 6.

And it also reports hundreds of null pointer dereferences caused by unchecked malloc failure. For example:
file: array.c
function: array_init_array
At line 32: a->data = malloc(...)
At line 34: if (src->data[i]) a->data[i] = src->data[i]->copy(src->data[i]);
use a->data without failure checking.

We could insert more assertions of course, but the result will be the same - lighttpd will die on OOM.

Hope for your replies! Thank you!

Regards,
Zhenbo Xu

Thank you very much for your input!

#2 Updated by zhenbxoxu 12 months ago

stbuehler wrote:

Hi!

zhenbxoxu wrote:

Hi,
I'm a developer of a static analysis tool canalyze.
Recently I applied it to lighttpd-1.4.32.

Thanks :)

It seems some reports are real after by manually checing:
1. Use Undefined Value
file: mod_redirect.c
function: SETDEFAULTS_FUNC
At line 86: data_array *da = (data_array *)du;
du is undefined value.

Yes... otoh, the assigned undefined value is never actually used.

2. Use Undefined Value
file: http_auth.c
function: base64_decode
At line 95: buffer_prepare_copy(out, in_len);
out->ptr may be a newly allocated memory object.

At line 109: if (ch < 0) continue;
ch < 0 is satifiable at the first iteration, i.e. i 0

And at the second iteration, i 1
At line 116: result[j++] |= ch >> 4;
result[j] is a undefined value.

Oh this looks ugly...
As it skips invalid characters (I think we could just fail on invalid characters) it should keep the "state % 4" in a separate var.
Also the padding seems to insert a 0 character as data (it counts to out->used).

I cannot see any insertion in the for loop. Could you explain it in more detail?

3. Memory leak
file: mod_simple_vhost.c
function: build_doc_root
At line 166: Do we need to free the buffer in out?

I don't see a buffer allocation, why would we free one? The buffer gets reused, no need to empty it every time.

4. Memory leak
file: proc_open.c
function: proc_open_buffer
At line 312: buffer *tmp = buffer_init();
Create a new buffer here.

At line 320: Do we need to free the buffer when error writing pipe.

We could. Otoh, the error in this place will make lighttpd exit anyway (only used for config include_shell).

5. Memory leak
file: fdevent.c
function: fdevent_init
At line 68: if (0 != fdevent_libev_init(ev))
"ev" is not released when event-handler libev failed.

Same as 4 - without an event-handler lighttpd won't start.

6. Memory leak
file: lemon.c
function: tplt_open
At line 2867: tpltname = pathsearch(lemp->argv0,lemp->tmplname,0);
Memory is allocatd here but not released when return.

Just a parser generator, only used for building. There are probably newer versions of lemon.

7. Use After free
file: mod_fastcgi.c
function: fastcgi_get_packet
At line 2426: buffer_free(packet->b);
Buffer is freed and is used
At line 2429: log_error_write(srv, FILE, LINE, "sdsds", "FastCGI: header too small:", packet->b->used, "bytes <", sizeof(FCGI_Header), "bytes, waiting for more data");

Right. Only debug option and FastCGI backends have other ways to cause DoS (OOM trigger by sending large responses to slow clients) => not a security issue.

8. Memory leak
file: network_linux_sendfile.c
function: network_write_chunkqueue_linuxsendfile
At line 180: if (HANDLER_ERROR == stat_cache_get_entry(srv, con, c->file.name, &sce)) {
Memory is allocated to sce,

Should we release "sce" at line 189 and 193?

No. stat cache entries are kept in a tree for later reuse. The mainloop triggers cleaning each second.

9. Memory leak
file: lemon.c
function: FindStates
At line 744: (void)getstate(lemp);
would return a heap object

See 6.

And it also reports hundreds of null pointer dereferences caused by unchecked malloc failure. For example:
file: array.c
function: array_init_array
At line 32: a->data = malloc(...)
At line 34: if (src->data[i]) a->data[i] = src->data[i]->copy(src->data[i]);
use a->data without failure checking.

We could insert more assertions of course, but the result will be the same - lighttpd will die on OOM.

OOM? Not Segment Fault?

Hope for your replies! Thank you!

Regards,
Zhenbo Xu

Thank you very much for your input!

#3 Updated by stbuehler 12 months ago

2. Use Undefined Value
file: http_auth.c
function: base64_decode
At line 95: buffer_prepare_copy(out, in_len);
out->ptr may be a newly allocated memory object.

At line 109: if (ch < 0) continue;
ch < 0 is satifiable at the first iteration, i.e. i 0

And at the second iteration, i 1
At line 116: result[j++] |= ch >> 4;
result[j] is a undefined value.

Oh this looks ugly...
As it skips invalid characters (I think we could just fail on invalid characters) it should keep the "state % 4" in a separate var.
Also the padding seems to insert a 0 character as data (it counts to out->used).

I cannot see any insertion in the for loop. Could you explain it in more detail?

The padding insert is at the end, not in the loop. Just comments to myself what i should fix :)

And it also reports hundreds of null pointer dereferences caused by unchecked malloc failure. For example:
file: array.c
function: array_init_array
At line 32: a->data = malloc(...)
At line 34: if (src->data[i]) a->data[i] = src->data[i]->copy(src->data[i]);
use a->data without failure checking.

We could insert more assertions of course, but the result will be the same - lighttpd will die on OOM.

OOM? Not Segment Fault?

OOM == Out of Memory. And either it dies with an abort() from the assert() or with a segfault - not a huge difference (as long as it actually segfaults; NULL from alloc could also lead to memory corruption if you only use memory at high offsets from the base pointer).

#4 Updated by stbuehler 11 months ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2872.

Also available in: Atom