Skip to content
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

Improve API Documentation #439

Closed
icy17 opened this issue Jun 12, 2024 · 5 comments
Closed

Improve API Documentation #439

icy17 opened this issue Jun 12, 2024 · 5 comments
Labels
documentation Documentation is incomplete, inaccurate, or unclear. feedback Waiting for feedback from submitter.

Comments

@icy17
Copy link

icy17 commented Jun 12, 2024

Hi, I've observed that some security rules are absent from the documentation, such as for API zip_fclose, which requires that Caller MUST NOT use the zf parameter after calling zip_fclose. Omitting these rules could result in overlooked necessary checks when using the API. Should I report these types of rules to your team?

@icy17 icy17 added the enhancement Request a new feature. label Jun 12, 2024
@dillof dillof added documentation Documentation is incomplete, inaccurate, or unclear. and removed enhancement Request a new feature. labels Jun 12, 2024
@dillof
Copy link
Member

dillof commented Jun 12, 2024

You mean we should document these in the man pages? Yes, I agree. Please report them as issues, or append them to this issue.

thanks,
dillo

@0-wiz-0 0-wiz-0 added the feedback Waiting for feedback from submitter. label Jul 11, 2024
@icy17
Copy link
Author

icy17 commented Jul 21, 2024

I've observed that some security rules are absent from the documentation.

For the security rule "parameters must not be NULL," its omission in the documentation can lead to significant security issues.

Firstly, while best programming practices dictate that developers should avoid passing NULL parameters to an API, this guideline may not be obvious to less experienced developers, increasing the risk of overlooking necessary security checks.

Secondly, many APIs receive parameters from other functions or APIs, which might not be adequately checked for NULL, posing a risk even for experienced developers.

Therefore, it is crucial to explicitly mention in the documentation that NULL checks must be conducted on specified parameters before an API is used. Including these security rules will help developers identify potential security issues more effectively during testing.

I will provide a list of APIs requiring checks along with their corresponding parameters:

API parameter index (begin with 1)
zip_stat_init 1
zip_error_code_zip 1
zip_error_init 1
zip_error_fini 1
zip_stat_index 4
zip_stat_index 1
zip_source_read 1
zip_source_stat 1
zip_error_code_system 1
zip_source_close 1
zip_source_tell_write 1
zip_error_to_data 2
zip_source_tell 1
zip_error_strerror 1
zip_file_attributes_init 1
zip_fopen_index 1

@icy17
Copy link
Author

icy17 commented Jul 21, 2024

There are other types of security rules that are missing in the documentation. I believe incorporating these rules could help developers learn how to correctly use APIs and reduce the risk of security issues.

I will provide a table describing the missing security rules:

API parameter index (begin with 1) Security rules
zip_error_fini 1 parameter must not be used later (without re-initialize)
zip_close 1 parameter must not be used later (without re-initialize)
zip_source_free 1 parameter must not be used later (without re-initialize)
zip_source_free 1 Parameter 1: Do not call zip_source_free multiple times for the same src parameter
zip_close 1 Parameter 1: The caller MUST NOT free the za parameter before calling zip_close
zip_close 1 parameter must be initialized using xxx (other API)
zip_name_locate 1 parameter must be initialized using xxx (other API)

@icy17
Copy link
Author

icy17 commented Jul 29, 2024

Hi, I'm not sure how to submit patches to supplement the documentation, so I'll provide some types of rules I've written based on other documentations. I'm not certain if they are appropriate, and if you find them acceptable, I can provide additional rules for other APIs in the same manner.
For API zip_close: The archive should be obtained from an API like zip_open, and not previously closed. The caller should not attempt to access or dereference the archive after invoking zip_close.
For API zip_source_read: The first parameter source must not be NULL.

@dillof dillof changed the title New security rules Improve API Documentation Aug 21, 2024
@dillof
Copy link
Member

dillof commented Aug 21, 2024

Some of your observations should be obvious to a programmer using a C API, some of them are wrong, as they are even contradicted by the existing documentation. I've added a note to libzip(3) that NULL pointers are not allowed unless explicitly documented, which is the general rule for C APIs.

@dillof dillof closed this as completed Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation is incomplete, inaccurate, or unclear. feedback Waiting for feedback from submitter.
Projects
None yet
Development

No branches or pull requests

3 participants