Welcome Guest, Not a member yet? Register   Sign In
Return values
#1

[eluser]Aethedor[/eluser]
If done some (quick) code auditing and seen that on several places, the return values of called functions are not handled properly. Specially towards error values. Below is a list of my findings of the files in /libararies.


libraries/Email.php
attach(), line 368
specs say @return string, function returns NULL
validate_email(), line 684
function returns FALSE on error, but NULL on success. should be TRUE.
function is called on several places in class, but return value is not checked.
_build_message(), line 959
function returns FALSE on error, but NULL on success. should be TRUE.
function is called on several places in class, but return value is not checked.
_spool_email(), line 1370
function is called on line 1327, but return value is not checked.
_smtp_connect(), line 1532
function returns FALSE on error, but NULL on success. should be TRUE.
_send_command(), line 1560
function is called on several places in class, but return value is not checked.
function is called on line 1476, but return value is not checked.
_smtp_authenticate(), line 1625
function is called on line 1477, but return value is not checked.
_send_data(), line 1679
function is called on several places in class, but return value is not checked.

libraries/Ftp.php
delete_dir(), line 370
function should check return value of delete_dir on line 390 and return FALSE on error
mirror(), line 482
return value of mirror() (line 507) not checked for errors. return FALSE on error.
return value of upload() (line 515) not checked for errors. return FALSE on error.

libraries/Hook.php
_call_hook(), line 91
return value of _run_hook() (line 102, 107) not checked for errors. return FALSE on error.
specs say @return mixed, function returns boolean.
_run_hook(), line 124
return value on line 140 is not a boolean

libraries/Image_lib.php
initialize(), line 128
specs say @return void, function returns boolean.
image_process_netpbm(), line 628
return value of copy() (line 696) and unlink() (line 697) not checked for errors, return FALSE on error.
size_calculator(), line 1383
return value NULL should be FALSE (line 1386).

libraries/Language.php
load(), line 52
specs say @return void, function returns TRUE and string
not defined variable used ($lang) on line 86

libraries/Loader.php
library(), line 74
function returns FALSE on error, but NULL on success. should be TRUE. (specs even say @return void)
database(), line 194
function should return object, but at the end of the function it returns NULL.
dbutil(), line 231
specs say @return string, function returns NULL.
dbforge(), line 261
specs say @return string, function returns NULL.
view(), line 298
specs say @return void, function returns !void;
_ci_load(), line 630
specs say @return void, function returns string (line 720).
_ci_autoloader(), 889,
specs say @return void, function returns FALSE on error (line 895).

libraries/Output.php
_write_cache(), line 281
return values of flock and fwrite not checked for errors (line 307).
_display_cache(), line 324
specs say @return void, function retuns boolean.
return value of flock not checked for errors, line 350.

libraries/URI.php
_fetch_uri_string(), line 60
specs say @return string, function returns NULL

libraries/User_agent.php
_compile_data(), line 133
specs say @return bool, function returns NULL
call to function should check for error code, line 70

libraries/Session.php
sess_read(), line 183
specs say @return void, function returns boolean.

libraries/Validation.php
set_select(), line 648
function returns NULL at the end, not ''.
set_radio(), line 674
function returns NULL at the end, not ''.

libraries/Zip.php
read_dir(), line 236,
returns NULL on opendir() error.
archive(), line 296
return value of flock() and fwrite() not checked.
#2

[eluser]Aethedor[/eluser]
Any of the CI developers who want to respond to this?
#3

[eluser]Aethedor[/eluser]
Is the CI projectteam going to take security serious or not? Otherwise I will stop trying to improve the quality of the code.
#4

[eluser]xwero[/eluser]
Most your remarks have to do with a wrong code documentation, right?

For the Language library i can say that the $lang variable is in the language file, so if it's defined in the library developers will not know there is something wrong with the language file they want to load.

For the Validation library when will the methods return NULL?

You should have some patience the EL developers are hard at work to get their bread and butter application out of the door which is already late.
#5

[eluser]Aethedor[/eluser]
[quote author="xwero" date="1223389013"]Most your remarks have to do with a wrong code documentation, right?[/quote]
No, it's about inconsistancy in the return values. It's about not getting back from a function what you expect, which can result in undesired code behaviour, which can result into a security problem.

Quote:For the Language library i can say that the $lang variable is in the language file, so if it's defined in the library developers will not know there is something wrong with the language file they want to load.
Should the function not check $lang is an array after the language file is included and raise an error if it's not? It now looks a bit hackish to me. These kind of checks make CodeIgniter more robust.

Quote:For the Validation library when will the methods return NULL?
When the 2 if-statements result into a false. The function just ends without a return, therefor returning null (void).

Quote:You should have some patience the EL developers are hard at work to get their bread and butter application out of the door which is already late.
Oke, but after 4 days I expected at least a "we're busy, we'll get back to you" kind of reply. The forum now looks pretty much dead to me...
#6

[eluser]Aethedor[/eluser]
There seems to be something wrong with this forum. It's the second time I pressed the [Submit Post] button while my post got lost....
#7

[eluser]xwero[/eluser]
You are right about an is_array check for the $lang variable but the isset check is to see if there is a $lang variable present so it has to come before the array check.
#8

[eluser]Derek Allard[/eluser]
Aethedor, I'd repeat the call for patience please. Which of the issues that you've identified do you feel are security issues, and why? I assure you that we take security very seriously. I recognize that some of the code comments you've identified are wrong (thanks) but I have to move these to a lower priority then security.
#9

[eluser]Aethedor[/eluser]
All the issues are inconsistancy in function declaration and usage of return values. This can lead to incorrect error checking when using these functions. Incorrect checking for error values (or not checking at ) when calling functions is a major cause of security issues.

I'm sure there are a lot of excuses why this isn't a security thing, but I don't care about those. I've seen many bad/non-error checking result into big security issues. The only thing I'm gonna do is share my experiences and knowledge about IT security with you to help you improve CodeIgniter's code quality. It's up to you what to do with it.
#10

[eluser]Aethedor[/eluser]
Seriously guys, fix your forum. This is the third time I enter a reply, press submit and all I end up with is an empty inputform and no reply submitted..... I'm not going to enter every post on this forum twice.....




Theme © iAndrew 2016 - Forum software by © MyBB