-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security] cURL SSL/TLS Verification -> option; Verbose Logging -> option/disabled #47
Comments
Thanks. On-going pull request #42 aims to improve on this in several ways. It should be opt-in to log at debug level for instance - option in proxmox.php |
As for the cURL options, that's to ensure compatibility with various PVE node crypto config states. I don't see a viable way we can change that, except for - as with debug-level logging - making it an option. Please let us know your thoughts @Anuril so the maintainers can consider this in the open PR #42 - thank you! |
I found several security flaws in the action function.
SSL/TLS Verification Disabled
https://github.com/CpuID/pve2-api-php-client/blob/b9ef9cb4f040cf71bfa5281b1e31bfae7a984474/pve2_api.class.php#L219C20-L219C20
This makes the requests vulnerable to Man-in-the-Middle (MITM) attacks, where an attacker can intercept and potentially alter the communication between the client and the server.
Potential Information Disclosure & Lack of Data Sanitization
Also described in #45
The function logs extensive details about the responses it receives, including headers and potentially sensitive data. If the logs are improperly secured or misconfigured, it could lead to information disclosure. Below is the code segment responsible for this:
pve2-api-php-client/pve2_api.class.php
Line 233 in b9ef9cb
The data received from the API response in the action function is directly logged and used without any visible sanitization or validation, making it potentially susceptible to security vulnerabilities, especially if this data is used elsewhere in the application.
Potential Inconsistent Return Types
For “PUT” HTTP method, it returns a Boolean true if the HTTP response is 200, but for other HTTP methods, it returns the ‘data’ part of the response. This inconsistency might lead to bugs or unexpected behavior in the parts of the application that use this function, indirectly causing security flaws.
In addition to the observed flaws, manually parsing HTTP responses, as seen in this function, is not a best practice, as it is error-prone and can lead to security vulnerabilities; it is generally safer and more efficient to use well-established libraries or built-in functions for processing HTTP responses, which are designed to handle various edge cases and anomalies in a secure manner.
The text was updated successfully, but these errors were encountered: