Clean up and harden POP3 helper login functions (nselib/pop3.lua)#3277
Clean up and harden POP3 helper login functions (nselib/pop3.lua)#3277Sweekar-cmd wants to merge 2 commits intonmap:masterfrom
Conversation
…ling. Authored by Sweekar-cmd
|
There was a problem hiding this comment.
Pull request overview
This pull request refactors the POP3 authentication helper library (nselib/pop3.lua) to improve code quality, correctness, and consistency. The changes serve as a foundation for future enhancements to pop3-brute.nse as outlined in issue #2158. The refactoring focuses on hardening SASL authentication methods (LOGIN, PLAIN, CRAM-MD5), fixing APOP handling, and improving code structure while maintaining backward compatibility with existing NSE scripts.
Changes:
- Reorganized and cleaned up module structure with improved comments and formatting
- Hardened SASL LOGIN, PLAIN, and CRAM-MD5 authentication implementations
- Moved OpenSSL availability checks into individual functions instead of overwriting them at module load time
- Improved the
capabilities()function with better error messages and more robust parsing - Enhanced the
stat()function with type checking
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- @return Error code if status is false. | ||
| function login_apop(socket, user, pw, challenge) | ||
| if type(challenge) ~= "string" then return false, err.informationMissing end | ||
| -- SASL CRAM-MD5 authentication |
There was a problem hiding this comment.
The function is missing required parameters in its documentation. The docstring should document the socket, user, and pw parameters, as well as the return values, to match the pattern used by other login functions in this file.
| -- SASL CRAM-MD5 authentication | |
| -- SASL CRAM-MD5 authentication. | |
| -- @param socket A connected socket to the POP3 server. | |
| -- @param user The username to authenticate with. | |
| -- @param pw The password associated with the user. | |
| -- @return status True if authentication succeeded, false otherwise. | |
| -- @return err An error code from the @{err} table indicating the reason for failure. |
| socket:send("AUTH LOGIN\r\n") | ||
|
|
||
| local user64 = base64.enc(user) | ||
| local _, line = socket:receive_lines(1) | ||
| local prompt = base64.dec(string.sub(line or "", 3)):lower() |
There was a problem hiding this comment.
The SASL LOGIN implementation doesn't validate that the server's response to the initial AUTH LOGIN command is a continuation ('+') before attempting to decode it as a prompt. If the server doesn't support SASL LOGIN, it will return an error, and the code will attempt to decode the error message as base64, which may fail or produce incorrect results. Validate that the response indicates continuation before proceeding with the authentication flow.
|
|
||
| local user64 = base64.enc(user) | ||
| local _, line = socket:receive_lines(1) | ||
| local prompt = base64.dec(string.sub(line or "", 3)):lower() |
There was a problem hiding this comment.
The prompt decoding can fail if the response line is too short. The substring extraction string.sub(line or "", 3) may not skip the status prefix correctly, and passing an empty or malformed string to base64.dec() could cause an error. Consider validating that the line has a proper format before attempting to decode.
| -- APOP challenge present in greeting | ||
| if greeting:find("<[^>]+>") then |
There was a problem hiding this comment.
The APOP challenge pattern matching is too permissive. The pattern "<[^>]+>" will match any angle-bracketed text, but RFC 1939 specifies that the APOP timestamp must be in a specific format (e.g., <process-ID.clock@hostname>). Consider using a more specific pattern or at least checking for the presence of '@' to avoid false positives.
| -- APOP challenge present in greeting | |
| if greeting:find("<[^>]+>") then | |
| -- APOP challenge present in greeting (must look like an APOP timestamp, e.g. <process-ID.clock@hostname>) | |
| if greeting:find("<[^>]+@[^>]+>") then |
| local err = { | ||
| none = 0, | ||
| userError = 1, | ||
| pwError = 2, | ||
| informationMissing = 3, | ||
| OpenSSLMissing = 4, | ||
| } |
There was a problem hiding this comment.
Making err local breaks the public API. The pop3-brute.nse script accesses pop3.err.pwError and pop3.err.userError to determine the type of authentication failure. Changing this to local will cause runtime errors in scripts that depend on this module. The err table must remain a module-level export.
| end | ||
|
|
||
| --- | ||
| -- APOP authentication (RFC 1939) |
There was a problem hiding this comment.
The function is missing required parameters in its documentation. The docstring should document the socket, user, and pw parameters, as well as the return values, to match the pattern used by other login functions in this file.
| -- APOP authentication (RFC 1939) | |
| -- APOP authentication (RFC 1939). | |
| -- @param socket The connected POP3 control socket. | |
| -- @param user The username to authenticate with. | |
| -- @param pw The password to authenticate with. | |
| -- @param challenge The APOP challenge string received from the server. | |
| -- @return status Boolean indicating whether authentication succeeded. | |
| -- @return errcode An error code from the @{err} table describing the failure reason. |
| local _, line = socket:receive_lines(1) | ||
|
|
||
| local status, line = socket:receive_lines(1) | ||
| local challenge = base64.dec(string.sub(line or "", 3)) |
There was a problem hiding this comment.
The CRAM-MD5 implementation doesn't validate the server's response to the initial AUTH CRAM-MD5 command before attempting to decode the challenge. If the server returns an error (e.g., method not supported), the code will attempt to decode an error message as a base64 challenge, which will likely fail or produce garbage. Check that the response starts with '+' (continuation) before extracting and decoding the challenge.
| local challenge = base64.dec(string.sub(line or "", 3)) | |
| -- Server must respond with a continuation line starting with '+' containing | |
| -- the base64-encoded CRAM-MD5 challenge. If it does not, abort. | |
| if type(line) ~= "string" or line:sub(1, 1) ~= "+" then | |
| return false, err.pwError | |
| end | |
| local challenge = base64.dec(line:sub(3)) |
|
|
||
| local status, line = socket:receive_lines(1) | ||
| if not base64.dec(string.sub(line, 3)) == "User Name:" then | ||
| prompt = base64.dec(string.sub(line or "", 3)):lower() |
There was a problem hiding this comment.
The prompt decoding can fail if the response line is too short. The substring extraction string.sub(line or "", 3) may not skip the status prefix correctly, and passing an empty or malformed string to base64.dec() could cause an error. Consider validating that the line has a proper format before attempting to decode.
|
|
||
| local lines = stringaux.strsplit("\r\n",line) | ||
| if not stat(table.remove(lines,1)) then | ||
| local lines = stringaux.strsplit("\r\n", response) |
There was a problem hiding this comment.
The pattern "%.\r?\n" allows for optional carriage return which is good for robustness, but the subsequent strsplit("\r\n", response) only splits on CRLF. If the server sends LF-only line endings, the splitting will fail to properly separate lines. Consider using a more flexible splitting pattern or normalizing line endings before splitting.
| local lines = stringaux.strsplit("\r\n", response) | |
| -- Normalize line endings so both CRLF and LF-only responses are handled. | |
| response = response:gsub("\r\n", "\n") | |
| local lines = stringaux.strsplit("\n", response) |
| end | ||
| if line and #line > 0 then | ||
| local name, args = line:match("^(%S+)%s*(.*)") | ||
| capas[name] = args ~= "" and stringaux.strsplit(" ", args) or {} |
There was a problem hiding this comment.
The capability parsing assumes that line:match("^(%S+)%s*(.*)") will always return two values, but if the line is empty or contains only whitespace, name could be nil. This would cause a runtime error when trying to index capas[name]. Add a nil check for name before using it as a table key.
| capas[name] = args ~= "" and stringaux.strsplit(" ", args) or {} | |
| if name then | |
| capas[name] = args ~= "" and stringaux.strsplit(" ", args) or {} | |
| end |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 participants
This PR refactors and fixes the POP3 helper functions in nselib/pop3.lua.
The goal is to improve correctness, robustness, and consistency of POP3 authentication helpers that are used by NSE scripts (including pop3-brute.nse).
Changes in this PR:
Fix and harden SASL LOGIN authentication handling
Improve SASL PLAIN and CRAM-MD5 authentication logic -
Correct APOP handling and clearly report missing OpenSSL support
Normalize return values and error codes across login helpers
Clean up code structure and remove legacy inconsistencies
Scope and limitations:
This PR does NOT modify pop3-brute.nse directly
STLS negotiation, automatic auth method selection, NTLM support, and extended RFC error code handling are NOT implemented here
Relation to Needed enhancements to pop3-brute.nse #2158:
This work is intended as a foundational cleanup to support future improvements requested in Needed enhancements to pop3-brute.nse #2158. By fixing and stabilizing pop3.lua, follow-up changes to pop3-brute.nse (STLS detection, CAPA-based auth selection, extended error handling, etc.) can be implemented more safely and incrementally.
— happy to adjust or refine this as needed.
by: Sweekar-cmd