Security Code Review

Security Code Review Guidelines

When reviewing code for security issues, systematically check for common vulnerabilities and suggest secure alternatives.

OWASP Top 10 Focus Areas

1. Injection Attacks (SQL, NoSQL, Command, LDAP)

Look for:

• String concatenation in SQL queries
• Unsanitized user input in database queries
• Direct execution of user input

Vulnerable:

// SQL Injection
String query = "SELECT * FROM users WHERE username = '" + username + "'";
Statement stmt = connection.createStatement();
ResultSet rs = stmt.executeQuery(query);

// Command Injection
Runtime.getRuntime().exec("ping " + userInput);

Secure:

// Use Prepared Statements
String query = "SELECT * FROM users WHERE username = ?";
PreparedStatement pstmt = connection.prepareStatement(query);
pstmt.setString(1, username);
ResultSet rs = pstmt.executeQuery();

// Avoid direct command execution; use APIs instead
// If unavoidable, validate and sanitize input
List<String> allowedHosts = Arrays.asList("localhost", "example.com");
if (allowedHosts.contains(userInput)) {
    // proceed
}

2. Broken Authentication

Look for:

• Passwords stored in plain text
• Weak password requirements
• Missing session timeout
• Predictable session IDs
• Missing multi-factor authentication

Vulnerable:

// Plain text password
user.setPassword(password);

// Weak session ID
String sessionId = user.getId() + System.currentTimeMillis();

Secure:

// Hash passwords with bcrypt
BCryptPasswordEncoder encoder = new BCryptPasswordEncoder();
String hashedPassword = encoder.encode(password);
user.setPassword(hashedPassword);

// Use cryptographically secure random session IDs
String sessionId = UUID.randomUUID().toString();

3. Sensitive Data Exposure

Look for:

• Logging sensitive information
• Transmitting data over HTTP
• Storing secrets in code
• Returning detailed error messages

Vulnerable:

// Logging sensitive data
logger.info("User password: " + password);

// Hardcoded secrets
String apiKey = "sk_live_1234567890abcdef";

// Detailed error messages
catch (Exception e) {
    return "Database error: " + e.getMessage();
}

Secure:

// Mask sensitive data in logs
logger.info("User authenticated: " + username);

// Use environment variables
String apiKey = System.getenv("API_KEY");

// Generic error messages for users
catch (Exception e) {
    logger.error("Database error", e); // Log details internally
    return "An error occurred. Please try again later.";
}

4. XML External Entities (XXE)

Look for:

• XML parsers without XXE protection
• Processing untrusted XML data

Vulnerable:

DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
DocumentBuilder builder = factory.newDocumentBuilder();
Document doc = builder.parse(userProvidedXml);

Secure:

DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
DocumentBuilder builder = factory.newDocumentBuilder();
Document doc = builder.parse(userProvidedXml);

5. Broken Access Control

Look for:

• Missing authorization checks
• Insecure direct object references
• Path traversal vulnerabilities
• CORS misconfiguration

Vulnerable:

// No authorization check
@GetMapping("/users/{id}")
public User getUser(@PathVariable Long id) {
    return userRepository.findById(id);
}

// Path traversal
File file = new File("/uploads/" + filename);

Secure:

// Check authorization
@GetMapping("/users/{id}")
public User getUser(@PathVariable Long id, Principal principal) {
    User currentUser = getCurrentUser(principal);
    if (!currentUser.canAccess(id)) {
        throw new AccessDeniedException("Unauthorized");
    }
    return userRepository.findById(id);
}

// Validate and sanitize file paths
Path basePath = Paths.get("/uploads").toAbsolutePath().normalize();
Path filePath = basePath.resolve(filename).normalize();
if (!filePath.startsWith(basePath)) {
    throw new SecurityException("Invalid file path");
}

6. Security Misconfiguration

Look for:

• Debug mode in production
• Default credentials
• Unnecessary features enabled
• Missing security headers
• Verbose error messages

Vulnerable:

# application.yml
spring:
  profiles:
    active: dev
debug: true

Secure:

# application-prod.yml
spring:
  profiles:
    active: prod
debug: false

# Add security headers
server:
  servlet:
    session:
      cookie:
        secure: true
        http-only: true

7. Cross-Site Scripting (XSS)

Look for:

• Unescaped user input in HTML
• innerHTML usage with user data
• Dangerous template rendering

Vulnerable:

// Reflected XSS
document.getElementById('greeting').innerHTML =
    "Hello " + userInput;

// DOM-based XSS
element.innerHTML = location.hash.substring(1);

Secure:

// Escape user input
document.getElementById('greeting').textContent =
    "Hello " + userInput;

// Use safe methods
const div = document.createElement('div');
div.textContent = userInput;
element.appendChild(div);

8. Insecure Deserialization

Look for:

• Deserializing untrusted data
• Using vulnerable serialization libraries

Vulnerable:

ObjectInputStream ois = new ObjectInputStream(userInputStream);
Object obj = ois.readObject(); // Dangerous!

Secure:

// Use safe alternatives like JSON
ObjectMapper mapper = new ObjectMapper();
MyObject obj = mapper.readValue(jsonString, MyObject.class);

// If ObjectInputStream required, validate class types
ObjectInputStream ois = new ObjectInputStream(userInputStream) {
    @Override
    protected Class<?> resolveClass(ObjectStreamClass desc)
            throws IOException, ClassNotFoundException {
        if (!desc.getName().equals("com.example.SafeClass")) {
            throw new InvalidClassException("Unauthorized deserialization");
        }
        return super.resolveClass(desc);
    }
};

9. Using Components with Known Vulnerabilities

Look for:

• Outdated dependencies
• Unpatched libraries
• Dependencies with security advisories

Check:

# Maven
mvn versions:display-dependency-updates
mvn dependency-check:check

# npm
npm audit

# Python
pip-audit

Recommendation:

• Keep dependencies updated
• Monitor security advisories
• Use dependency scanning tools in CI/CD
• Remove unused dependencies

10. Insufficient Logging and Monitoring

Look for:

• No audit logs for security events
• Missing failed login tracking
• No alerting for suspicious activity

Vulnerable:

@PostMapping("/login")
public void login(String username, String password) {
    if (authenticate(username, password)) {
        // Login successful
    }
    // No logging
}

Secure:

@PostMapping("/login")
public void login(String username, String password, HttpServletRequest request) {
    boolean success = authenticate(username, password);

    if (success) {
        auditLog.info("Successful login: user={}, ip={}",
            username, request.getRemoteAddr());
    } else {
        auditLog.warn("Failed login attempt: user={}, ip={}",
            username, request.getRemoteAddr());
        failedLoginTracker.record(username, request.getRemoteAddr());
    }
}

Additional Security Checks

Cryptography

Look for:

• Use of weak algorithms (MD5, SHA1, DES)
• Hardcoded encryption keys
• Missing initialization vectors
• Weak random number generators

Vulnerable:

MessageDigest md = MessageDigest.getInstance("MD5");
Random random = new Random();
byte[] key = "hardcodedkey1234".getBytes();

Secure:

MessageDigest md = MessageDigest.getInstance("SHA-256");
SecureRandom random = new SecureRandom();
byte[] key = loadKeyFromSecureStorage();

API Security

Look for:

• Missing rate limiting
• No API authentication
• Excessive data exposure
• Missing input validation

Secure Practices:

@RestController
@RequestMapping("/api/v1")
public class UserController {

    // Rate limiting
    @RateLimiter(name = "userApi")
    @GetMapping("/users")
    public Page<UserDto> getUsers(
            @RequestParam(defaultValue = "0") int page,
            @RequestParam(defaultValue = "20") int size,
            Principal principal) {

        // Validate input
        if (size > 100) {
            throw new IllegalArgumentException("Page size too large");
        }

        // Return only necessary fields
        return userService.findAll(page, size)
            .map(this::toDto);
    }
}

Security Review Checklist

When reviewing code, check:

• Input validation on all user inputs
• Output encoding for all user-controlled data
• Parameterized queries for all database access
• Authentication on all protected resources
• Authorization checks before accessing resources
• Secure password storage (bcrypt, Argon2)
• HTTPS for all data transmission
• Security headers (CSP, X-Frame-Options, etc.)
• Error handling that doesn't leak information
• Logging of security-relevant events
• Rate limiting on public APIs
• CSRF protection for state-changing operations
• Updated dependencies without known vulnerabilities
• Secrets stored in environment variables or vault
• Proper session management and timeout

Reporting Security Issues

When documenting security findings:

### [CRITICAL] SQL Injection in User Search

**Location**: UserController.java:45

**Issue**: User input is concatenated directly into SQL query, allowing SQL injection attacks.

**Vulnerable Code**:
```java
String query = "SELECT * FROM users WHERE name LIKE '%" + searchTerm + "%'";

Impact: Attackers can execute arbitrary SQL, potentially accessing all database data or modifying records.

Recommendation: Use parameterized queries with PreparedStatement.

Fixed Code:

String query = "SELECT * FROM users WHERE name LIKE ?";
PreparedStatement stmt = connection.prepareStatement(query);
stmt.setString(1, "%" + searchTerm + "%");

Severity: Critical CVSS Score: 9.8 (Critical) Remediation Priority: Immediate

## When This Skill Activates

This skill automatically activates when:
- Reviewing code for security vulnerabilities
- Performing security audits
- Analyzing authentication/authorization code
- Checking for OWASP Top 10 vulnerabilities
- Questions about secure coding practices
- Reviewing API security
- Analyzing cryptographic implementations