Modify ↓

Ticket #92 (new task)

Opened 3 years ago

Last modified 10 days ago

audit du code

Reported by: thaeron Owned by: thaeron
Priority: minor Milestone: Version 1.7
Component: kernel Version:
Keywords: Cc:

Description

Juste avant la release 1.7 je prévois de faire un audit complet du bot (kernel + modules) afin de trouver et corriger tout bug ou vuln.
Une fois corrigé il sera soumis au module qui testera les non régessions.

Ca fait un gros taff, ceux qui veulent participer sont les bienvenus.

Attachments

Change History

comment:1 Changed 3 years ago by thaeron

xax m'a montré un problème potentiel avec strncpy (utilisée massivement dans NS), et qui doit donc être vérifiée.
Pour faire simple je vais citer la page de manuel : "Dans le cas où la longueur src est inférieure à n, la fin de dest sera remplie avec des caractères nuls."

Il faudra donc bien vérifier que n <= strlen (src)

comment:2 Changed 3 years ago by thaeron

Petite précision : ou bien sûr que la taille de dest soit supérieure ou égale à n.

comment:3 Changed 3 years ago by thaeron

Il existe un tas d'outils pour vérifier automatiquement le code. J'ai déjà commencé avec scan-build (qui vient du projet llvm-clang), il reste encore à essayer/utiliser : RATS, Sparse, splint, cppcheck et yasca.
scan-build n'a trouvé que affectations inutiles (et 2 erreurs qui n'en sont pas).

comment:4 Changed 3 years ago by thaeron

J'ai fait un petit test des différents outils pour voir un peu de quoi ils étaient capable. Voici le test : 

Source :
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void test_it (const char *sc, char *s)
{
	char *ptr;
	
	ptr = malloc (strlen (sc)); /* err 1 : strlen (sc) + 1 */
	strcpy (ptr, sc);	    /* warn : malloc might return NULL */
	
	ptr = strchr (s, 'k');	   /* err 2 : deferencing ptr memory */
	*ptr = '\0';		   /* err 3 : ptr can be NULL */
}
	


int main (void)
{
	char popo[10] = "youpie";
	
	test_it (popo, "cake"); /* err 4 : const char * assumed as char * which is truncated */
	
	return (0);
}



GCC :
[thaeron@syndrome Sources]$ gcc test_analyzer.c -o test_analyzer.c.x -ansi -pedantic -Wall
[thaeron@syndrome Sources]$

Valgrind :
[thaeron@syndrome Sources]$ valgrind  --leak-check=full --show-reachable=yes test_analyzer.c.x
==4475== Memcheck, a memory error detector.
==4475== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==4475== Using LibVEX rev 1854, a library for dynamic binary translation.
==4475== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==4475== Using valgrind-3.3.1, a dynamic binary instrumentation framework.
==4475== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==4475== For more details, rerun with: -v
==4475== 
==4475== Invalid write of size 1
==4475==    at 0x4C24168: strcpy (mc_replace_strmem.c:268)
==4475==    by 0x4005DD: test_it (in /home/thaeron/Sources/test_analyzer.c.x)
==4475==    by 0x400636: main (in /home/thaeron/Sources/test_analyzer.c.x)
==4475==  Address 0x5183036 is 0 bytes after a block of size 6 alloc'd
==4475==    at 0x4C21D1B: malloc (vg_replace_malloc.c:207)
==4475==    by 0x4005CC: test_it (in /home/thaeron/Sources/test_analyzer.c.x)
==4475==    by 0x400636: main (in /home/thaeron/Sources/test_analyzer.c.x)
==4475== 
==4475== Process terminating with default action of signal 11 (SIGSEGV)
==4475==  Bad permissions for mapped region at address 0x40072E
==4475==    at 0x4005F4: test_it (in /home/thaeron/Sources/test_analyzer.c.x)
==4475==    by 0x400636: main (in /home/thaeron/Sources/test_analyzer.c.x)
==4475== 
==4475== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 1)
==4475== malloc/free: in use at exit: 6 bytes in 1 blocks.
==4475== malloc/free: 1 allocs, 0 frees, 6 bytes allocated.
==4475== For counts of detected errors, rerun with: -v
==4475== searching for pointers to 1 not-freed blocks.
==4475== checked 74,032 bytes.
==4475== 
==4475== 
==4475== 6 bytes in 1 blocks are definitely lost in loss record 1 of 1
==4475==    at 0x4C21D1B: malloc (vg_replace_malloc.c:207)
==4475==    by 0x4005CC: test_it (in /home/thaeron/Sources/test_analyzer.c.x)
==4475==    by 0x400636: main (in /home/thaeron/Sources/test_analyzer.c.x)
==4475== 
==4475== LEAK SUMMARY:
==4475==    definitely lost: 6 bytes in 1 blocks.
==4475==      possibly lost: 0 bytes in 0 blocks.
==4475==    still reachable: 0 bytes in 0 blocks.
==4475==         suppressed: 0 bytes in 0 blocks.
Erreur de segmentation


scan-build :
[thaeron@syndrome Sources]$ scan-build gcc test_analyzer.c -o test_analyzer.c.x -ansi -pedantic -Wall
ANALYZE: test_analyzer.c test_it
ANALYZE: test_analyzer.c main
scan-build: Removing directory '/tmp/scan-build-2008-12-28-1' because it contains no reports.


splint :
[thaeron@syndrome Sources]$ splint -exportlocal test_analyzer.c
Splint 3.1.2 --- 28 Dec 2008

test_analyzer.c: (in function test_it)
test_analyzer.c:10:10: Possibly null storage ptr passed as non-null param:
                          strcpy (ptr, ...)
  A possibly null pointer is passed as a parameter corresponding to a formal
  parameter with no /*@null@*/ annotation.  If NULL may be used for this
  parameter, add a /*@null@*/ annotation to the function parameter declaration.
  (Use -nullpass to inhibit warning)
   test_analyzer.c:9:8: Storage ptr may become null
test_analyzer.c:12:2: Fresh storage ptr (type char *) not released before
                         assignment: ptr = strchr(s, 'k')
  A memory leak has been detected. Storage allocated locally is not released
  before the last reference to it is lost. (Use -mustfreefresh to inhibit
  warning)
   test_analyzer.c:9:2: Fresh storage ptr created
test_analyzer.c:13:3: Dereference of possibly null pointer ptr: *ptr
  A possibly null pointer is dereferenced.  Value is either the result of a
  function which may return null (in which case, code should check it is not
  null), or a global, parameter or structure field declared with the null
  qualifier. (Use -nullderef to inhibit warning)
   test_analyzer.c:12:8: Storage ptr may become null

Finished checking --- 3 code warnings


sparse :
[thaeron@syndrome Sources]$ sparse test_analyzer.c     
test_analyzer.c:5:6: warning: symbol 'test_it' was not declared. Should it be static?


cppcheck :
[thaeron@syndrome Sources]$ cppcheck --all test_analyzer.c
Checking test_analyzer.c...
No errors found


rats :
[thaeron@syndrome Sources]$ rats  --noheader --nofooter test_analyzer.c
Analyzing test_analyzer.c
test_analyzer.c:10: High: strcpy
Check to be sure that argument 2 passed to this function call will not copy
more data than can be handled, resulting in a buffer overflow.

test_analyzer.c:20: High: fixed size local buffer
Extra care should be taken to ensure that character arrays that are allocated
on the stack are used safely.  They are prime targets for buffer overflow
attacks.


--------------------
--- Code corrigé ---
--------------------
Source :
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void test_it (const char *sc, char *s)
{
	char *ptr;
	
	ptr = malloc (strlen (sc) + 1);
	if (ptr)
	{
		strcpy (ptr, sc);
		free (ptr);
	}
	
	ptr = strchr (s, 'k');	  
	if (ptr)
		*ptr = '\0';		   
}
	


int main (void)
{
	char popo[10] = "youpie", cake[] = "cake";
	
	test_it (popo, cake); 
	
	return (0);
}

GCC :
[thaeron@syndrome Sources]$ gcc test_analyzer.c -o test_analyzer.c.x -ansi -pedantic -Wall
[thaeron@syndrome Sources]$


Valgrind :
[thaeron@syndrome Sources]$ valgrind test_analyzer.c.x
==4699== Memcheck, a memory error detector.
==4699== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==4699== Using LibVEX rev 1854, a library for dynamic binary translation.
==4699== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==4699== Using valgrind-3.3.1, a dynamic binary instrumentation framework.
==4699== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==4699== For more details, rerun with: -v
==4699== 
==4699== 
==4699== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 1)
==4699== malloc/free: in use at exit: 0 bytes in 0 blocks.
==4699== malloc/free: 1 allocs, 1 frees, 7 bytes allocated.
==4699== For counts of detected errors, rerun with: -v
==4699== All heap blocks were freed -- no leaks are possible.


scan-build :
[thaeron@syndrome Sources]$ scan-build gcc test_analyzer.c -o test_analyzer.c.x -ansi -pedantic -Wall
ANALYZE: test_analyzer.c test_it
ANALYZE: test_analyzer.c main
scan-build: Removing directory '/tmp/scan-build-2008-12-28-1' because it contains no reports.


splint :
[thaeron@syndrome Sources]$ splint -exportlocal test_analyzer.c
Splint 3.1.2 --- 28 Dec 2008

Finished checking --- no warnings


sparse :
[thaeron@syndrome Sources]$ sparse test_analyzer.c
test_analyzer.c:5:6: warning: symbol 'test_it' was not declared. Should it be static?


cppcheck :
[thaeron@syndrome Sources]$ cppcheck --all test_analyzer.c
Checking test_analyzer.c...
No errors found


rats :
[thaeron@syndrome Sources]$ rats  --noheader --nofooter test_analyzer.c
Analyzing test_analyzer.c
test_analyzer.c:10: High: strcpy
Check to be sure that argument 2 passed to this function call will not copy
more data than can be handled, resulting in a buffer overflow.

test_analyzer.c:22: High: fixed size local buffer
Extra care should be taken to ensure that character arrays that are allocated
on the stack are used safely.  They are prime targets for buffer overflow
attacks.

Conclusion :
A part splint qui trouve 1 erreur et 1 warning sur 4 erreurs et 1 warning, tous les autres n'ont rien trouvé. Je pense donc que passer ces outils sur le code de NewSyndrome? serait une perte de temps. Cependant, concernant scan-build, c'est un projet très récent donc peut-être plus tard il sera capable de déceler des problèmes.
Valgrind a très bien vu le problème sur strcpy mais pas du tout sur le tronquage du const char*, et c'est justement pour ce genre de cas que je voulais utiliser un analyseur statique de code.

comment:5 Changed 3 years ago by timy_01

Je vais peut être dire une grosse bêtise mais pourquoi pas lancer un projet d'analyseur statique de code sur nos base à nous ?

Je suis sur que Dbd pourrait être intéressé, moi ca m'intéresse. Le seul problème qu'on pourrait avoir c'est "comment on code un analyseur statique de code ?"

Un idée ?

comment:6 Changed 3 months ago by gedsheeda

Abilify tablets for sale at Lewes

comment:7 Changed 2 months ago by Fafeasedlylef

buy benadryl -  http://www.flickr.com/people/70567851@N03/ buy benadryl online

View ↑

Add a comment

Modify Ticket

Action
as new
Author


E-mail address and user name can be saved in the Preferences.

Attachments ↑
 
Note: See TracTickets for help on using tickets.