Secure Coding Guidelines Fabrizio Pastore Università degli Studi di Milano -­‐ Bicocca Security Guidelines • Domain dependent guidelines – No AutomaBc Checks – Long et al., Java Coding Guidelines: 75 RecommendaBons for Reliable and Secure Programs • AutomaBc – FindBugs rules • More Guidelines – hNps://www.securecoding.cert.org/confluence/
display/java/The+CERT+Oracle+Secure+Coding
+Standard+for+Java Limit the lifeBme of sensiBve data • DaB sensibili potrebbero essere ispezionaB da un aNaccante se l’applicazione – usa oggeX per memorizzare daB sensibili, e gli oggeX non sono ripuliB/garbage collected – ha pagine di memoria che vengono swappate su disco – mostra i daB sensibili in messaggi di debugging Limit the lifeBme of sensiBve data • DaB sensibili potrebbero essere ispezionaB da un aNaccante se l’applicazione – usa oggeX per memorizzare daB sensibili, e gli oggeX non sono ripuliB/garbage collected – ha pagine di memoria che vengono swappate su disco – mostra i daB sensibili in messaggi di debugging • Soluzione: ripulire i daB Store Passwords using Hash FuncBons • Password salvate in plain text potrebbero essere osservate da un aNaccante • “Hash funcBons” computaBonally feasible funcBons whose inverses are computaBonally infeasible – evitare MD5 – preferire SHA256 Usare il SecurityManager char passwd[] = /* iniBalize */ try { System.setSecurityManager( new CustomSecurityManager( passwd ) ); } catch (SecurityExcepBon se ) { LOGGER.error( se, “Cannot set securityManager” ); } Minimizzare la visibilità delle Variabili NO SI public staBc void main(String args[]){ int i; for( i = 0; i < args.length; i++ ){ /* Do something*/ } } public staBc void main(String args[]){ for( int i = 0; i < args.length; i++ ){ /* Do something*/ } } Non dare per scontata l’esecuzione del costruNore • Diversi metodi per allocare una classe senza invocare costruNNore • Se inizializzazione è necessaria – usare una variabile di supporto “private”: ini#alized – seNare la variabile nel costruNore come ulBma istruzione – verificare in tuX i metodi che questa sia posta a true – se necesario usare anche classIni#alized Creare Classi non Serializzabili (se necessario) • La serializzazione in alcuni contesB potrebbe essere pericolosa perchè permeNe ad un aNaccante di accedere allo stato interno degli oggeX private final void writeObject(ObjectOutputStream out) throws java.io.IOExcepBon { throw new java.io.IOExcepBon("Object cannot be serialized"); } Rendere le Classi non Serializzabili anche Non Deserializzabili • Un aNaccante potrebbe creare un oggeNo ad doc che può essere deserializzato, pure per una classe non serializzabile! private final void readObject(ObjectInputStream in) throws java.io.IOExcepBon { throw new java.io.IOExcepBon("Class cannot be deserialized"); } Creare Classi Non Clonabili (se necessario) • Un aNaccante potrebbe evitare di eseguire il costruNore della vostra classe (che potrebbe contenere security checks ad esempio): – Un aNaccante può creare una soNoclasse che si limita ad implementare il metodo “clone” • Lanciare un eccezione: public final void clone() throws java.lang.CloneNotSupportedExcepBon { throw new java.lang.CloneNotSupportedExcepBon(); } Creare Classi Non Clonabili (se necessario) • Se cloning necessario: public final void clone() throws java.lang.CloneNotSupportedExcepBon { throw new java.lang.CloneNotSupportedExcepBon(); } GesBre Integer Overflow private void checkGrowBy(long extra) { if (extra < 0 || current > max -­‐ extra) { throw new IllegalArgumentExcepBon(); } } GesBre Integer Overflow (2) private void checkGrowBy(long extra) { BigInteger currentBig = BigInteger.valueOf(current); BigInteger maxBig = BigInteger.valueOf(max ); BigInteger extraBig = BigInteger.valueOf(extra ); if (extra < 0 || currentBig.add(extraBig).compareTo(maxBig) > 0) { throw new IllegalArgumentExcepBon(); } FindBugs • StaBc analysis tool • Console + GUI + Eclipse Plug-­‐in May expose internal representaBon by returning reference to mutable object • FindBugs: EI_EXPOSE_REP (EI) • Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representaBon of the object. • If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properBes, you will need to do something different. Returning a new copy of the object is beNer approach in many situaBons. May expose internal representaBon by incorporaBng reference to mutable object • FindBugs: EI_EXPOSE_REP2 (EI2) • This code stores a reference to an externally mutable object into the internal representaBon of the object. • If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properBes, you will need to do something different. Storing a copy of the object is beNer approach in many situaBons. Empty database password • FindBugs: DMI_EMPTY_DB_PASSWORD • This code creates a database connect using a blank or empty password. • This indicates that the database is not protected by a password. Hardcoded constant database password • FindBugs: DMI_CONSTANT_DB_PASSWORD • This code creates a database connect using a hardcoded, constant password. • Anyone with access to either the source code or the compiled code can easily learn the password. SQL InjecBon: A prepared statement is generated from a nonconstant String • Findbugs: SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING • The code creates an SQL prepared statement from a nonconstant String. • If the string is not checked, tainted data from a user is used in building this String. • SQL injecBon could be used to make the prepared statement do something unexpected and undesirable. SQL InjecBon: Nonconstant string passed to execute method on an SQL statement • FindBugs: SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE • The method invokes the execute method on an SQL statement with a String that seems to be dynamically generated. • Consider using a prepared statement instead. It is more efficient and less vulnerable to SQL injecBon aNacks. Field isn’t final but should be Field isn't final but should be refactored to be so • BugFinder: MS_SHOULD_BE_FINAL / MS_SHOULD_BE_REFACTORED_TO_BE_FINAL • This staBc field public but not final, and could be changed by malicious code or by accident from another package. • The field could be made final to avoid this vulnerability. • Refactoring is suggested if the staBc iniBalizer contains more than one write to the field, so doing so will require some refactoring. Field should be package protected • A mutable staBc field could be changed by malicious code or by accident. The field could be made package protected to avoid this vulnerability. Field is a mutable array/HashTable • FindBugs : MS_MUTABLE_ARRAY / MS_MUTABLE_HASHTABLE • A final staBc field references an array/Hashtable and can be accessed by malicious code or by accident from another package. • This code can freely modify the contents of the array/Hashtable. Method invoked that should only be invoked inside a doPrivileged block • FindBugs: DP_DO_INSIDE_DO_PRIVILEGED • This code invokes a method that requires a security permission check. • If this code will be granted security permissions, but might be invoked by code that does not have security permissions, then the invocaBon needs to occur inside a doPrivileged block. Classloaders should only be created inside doPrivileged block • FindBugs: DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVI
LEGED • This code creates a classloader, which needs permission if a security manage is installed. If this code might be invoked by code that does not have security permissions, then the classloader creaBon needs to occur inside a doPrivileged block. Finalizer should be protected, not public • FindBugs: FI_PUBLIC_SHOULD_BE_PROTECTED • A class's finalize() method should have protected access, not public. • Finalize method for any class can always invoke the finalize method for its superclass • Otherwise users can call finalize to free resources that are sBll referenced by someone Absolute/RelaBve path traversal in servlet • FindBugs: PT_ABSOLUTE_PATH_TRAVERSAL/ PT_RELATIVE_PATH_TRAVERSAL • The so|ware uses an HTTP request parameter to construct a pathname that should be within a restricted directory, but it does not properly neutralize sequences such as: – ".." can resolve to a locaBon that is outside of that directory. See hNp://cwe.mitre.org/data/definiBons/23.html – "/abs/path” can resolve to a locaBon that is outside of that directory. See hNp://cwe.mitre.org/data/definiBons/36.html for more informaBon. • FindBugs looks only for the most blatant, obvious cases of relaBve path traversal. If FindBugs found any, you almost certainly have more vulnerabiliBes that FindBugs doesn't report. FindBugs xml Filters • Filtering based on – bug CATEGORY / PATTERN_NAME / CODE_NAME – class name – confidence value • high-­‐confidence (1), normal-­‐confidence (2), low-­‐confidence (3) – rank value • scariest (1-­‐4), scary (5-­‐9), troubling (10-­‐14), concern (15-­‐20) – method name / param types /return type – field name / type – local var name •
•
•
•
•
•
•
•
•
•
MS: Field isn't final and can't be protected from malicious code (MS_CANNOT_BE_FINAL) A mutable staBc field could be changed by malicious code or by accident from another package. Unfortunately, the way the field is used doesn't allow any easy fix to this problem. MS: Public staSc method may expose internal representaSon by returning array (MS_EXPOSE_REP) A public staBc method returns a reference to an array that is part of the staBc state of the class. Any code that calls this method can freely modify the underlying array. One fix is to return a copy of the array. MS: Field should be both final and package protected (MS_FINAL_PKGPROTECT) A mutable staBc field could be changed by malicious code or by accident from another package. The field could be made package protected and/or made final to avoid this vulnerability. MS: Field is a mutable array (MS_MUTABLE_ARRAY) A final staBc field references an array and can be accessed by malicious code or by accident from another package. This code can freely modify the contents of the array. MS: Field is a mutable Hashtable (MS_MUTABLE_HASHTABLE) A final staBc field references a Hashtable and can be accessed by malicious code or by accident from another package. This code can freely modify the contents of the Hashtable. • May expose internal representaBon by returning reference to mutable object • May expose internal representaBon by incorporaBng reference to mutable object • May expose internal staBc state by storing a mutable object into a staBc field • Public staBc method may expose internal representaBon by returning array Method may fail to clean up stream or resource • FindBugs: OBL_UNSATISFIED_OBLIGATION • This method may fail to clean up (close, dispose of) a stream, database object, or other resource requiring an explicit cleanup operaBon. • If a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns. Method may fail to clean up stream or resource on checked excepBon • FindBugs: OBL_UNSATISFIED_OBLIGATION_EXCEPTION_EDGE • This method may fail to clean up (close, dispose of) a stream, database object, or other resource requiring an explicit cleanup operaBon. Method may fail to close database resource • FindBugs: ODR_OPEN_DATABASE_RESOURCE • The method creates a database resource (such as a database connecBon or row set), does not assign it to any fields, pass it to other methods, or return it, and does not appear to close the object on all paths out of the method. • Failure to close database resources on all paths out of a method may result in poor performance, and could cause the applicaBon to have problems communicaBng with the database.
