Fix issues reported by SonarCloud and add tests #617
Conversation
WalkthroughThe recent changes focus on enhancing access control mechanisms in the security components to improve security architecture. These modifications involve restructuring functions related to access control lists (ACLs), permission evaluation, and security state management. Additionally, a new file has been introduced to centralize ACL management and permission evaluation tasks, streamlining the overall security framework. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (55)
Files skipped from review as they are similar to previous changes (37)
Additional Context UsedAST-based Instructions (70)
Additional comments not posted (51)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (f57a3fe), label this PR with |
|
@coderabbitai create unit tests for |
|
Tip For best results, initiate chat on the files or code changes. @Danielius1922, based on the content of Unit Test Outline
Additional Considerations
Given the complexity of writing unit tests and the need for a deep understanding of the project's testing conventions and setup, I recommend reviewing the project's documentation on testing, looking at existing tests for examples, and possibly consulting with the project maintainers or contributors for guidance on writing consistent and comprehensive tests. Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Find and display the content of `util/oc_memb.c`
fd oc_memb.c util/ --exec cat {}
Length of output: 4588 |
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
f57a3fe to
e6a1160
Compare
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
e6a1160 to
3aa722b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- api/unittest/etagtest.cpp (2 hunks)
- docker/apps/Dockerfile.cloud-server (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug-clang (1 hunks)
- include/oc_rep.h (1 hunks)
- port/unittest/clocktest.cpp (1 hunks)
- security/unittest/keypairtest.cpp (1 hunks)
- util/oc_atomic.h (2 hunks)
- util/unittest/memblocktest.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- docker/apps/Dockerfile.cloud-server-debug
Additional comments: 17
docker/apps/Dockerfile.cloud-server (2)
- 1-1: Using a specific version of Alpine (
3.19) instead of the latest version can ensure more predictable and stable builds by avoiding unexpected changes in the base image. However, it's important to periodically review and update this to benefit from security patches and new features.- 14-14: Adding comments to clarify the purpose of specific installations or configurations, such as enabling
struct stat64, is a good practice for maintainability. It helps future developers understand the rationale behind certain decisions.docker/apps/Dockerfile.cloud-server-debug-clang (2)
- 6-7: Cleaning up packages after installation with
apt-get cleanis a good practice to reduce the image size. This step removes the package cache, which is not needed in the final image.- 9-11: Installing
clang-15and setting it as the default compiler usingupdate-alternativesis a clear and effective way to ensure that the desired compiler version is used. This approach also makes it easier to update or change the compiler in the future if needed.util/unittest/memblocktest.cpp (6)
- 34-43: The test case
Initcorrectly initializes a memory block and verifies its size and number of blocks based on theOC_DYNAMIC_ALLOCATIONflag. This is a good practice to ensure that the initialization logic works as expected under different configurations.- 46-49: The test case
Alloc_Failproperly handles the scenario where allocation should fail, i.e., when a null pointer is passed. This test ensures that the function behaves correctly in error conditions.- 65-68: The test case
Dealloc_Failcorrectly tests the scenario where deallocation should fail, i.e., when null pointers are passed. This ensures that the function behaves correctly in error conditions.- 70-90: The test case
AllocExceedsLimiteffectively tests the behavior of the allocation function when the limit of allocations is reached. It also correctly handles the difference in behavior based on theOC_DYNAMIC_ALLOCATIONflag. This test ensures that the memory block management system correctly enforces limits on allocations.- 94-105: The test case
Reallocis conditionally compiled for non-dynamic allocation scenarios and tests that a block can be reallocated after being freed. This ensures that the memory management system correctly handles block reuse. It's a good practice to include such conditionally compiled tests to cover different build configurations.- 107-122: The test case
InBlockcorrectly verifies whether allocated blocks belong to specific memory blocks. This test is crucial for ensuring the integrity of the memory block management system, especially when multiple memory blocks are in use.port/unittest/clocktest.cpp (1)
- 72-72: Converting the
secondsvariable to alongtype is a prudent change that ensures the variable can hold larger values, which might be necessary for certain platforms or scenarios. This change improves the robustness of the test.util/oc_atomic.h (2)
- 41-44: Converting macro definitions to typedefs for atomic integer types (
OC_ATOMIC_INT8_T,OC_ATOMIC_UINT8_T,OC_ATOMIC_INT32_T,OC_ATOMIC_UINT32_T) enhances clarity and type safety. This is a positive change that makes the code more readable and maintainable.- 83-86: Repeating the conversion of macro definitions to typedefs for atomic integer types in the Windows-specific section of the code ensures consistency across different platforms. This is a good practice that maintains clarity and type safety across the codebase.
security/unittest/keypairtest.cpp (1)
- 279-290: The modifications to the assertions within the
generate_keypairlambda function, specifically adjusting theEXPECT_EQandEXPECT_TRUEassertions, appear to streamline the test code and focus on the essential aspects of keypair generation and validation. Ensuring that the number of keypairs before and after the operation matches expected values is crucial for the integrity of the test.api/unittest/etagtest.cpp (2)
- 597-599: The simplification of the return statement in
isETagStorageEmptyfunction looks good and should maintain the original functionality. It's a good practice to simplify code for readability and maintainability.- 687-687: Changing the parameter type to
std::string_viewinisPlatformResourceURIfunction is a good optimization to avoid unnecessary string copies. Just ensure that the lifetime of the passed string always exceeds thestd::string_viewto prevent dangling references.include/oc_rep.h (1)
- 1192-1200: The introduction of the typedef union
oc_rep_value_tfor thevaluefield in theoc_rep_valuestruct is a positive change for readability and maintainability. Ensure that all parts of the project that interact with this struct are updated to use the newoc_rep_value_ttype.Verification successful
The examination of the occurrences of "oc_rep_value" in the project files indicates that the references are related to
oc_rep_value_type_t, not direct uses of the oldoc_rep_valueunion structure. This suggests that the project has been updated to use the newoc_rep_value_ttypedef union as intended. The initial review comment is thus validated by the absence of direct references to the old union structure, indicating that the changes have been correctly propagated throughout the project.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of the old `oc_rep_value` union to ensure they are updated. rg --type c "oc_rep_value" --files-with-matchesLength of output: 144
Script:
#!/bin/bash # Examine the occurrences of "oc_rep_value" in the listed files to understand their context. for file in include/oc_rep.h api/oc_rep.c api/oc_rep_internal.h api/oc_rep_decode_json.c api/oc_rep_decode.c; do echo "Occurrences in $file:" rg "oc_rep_value" "$file" echo "-----" doneLength of output: 2333
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
3aa722b to
b2470e9
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (25)
- api/oc_collection.c (1 hunks)
- api/oc_discovery.c (1 hunks)
- api/oc_ri.c (1 hunks)
- api/unittest/etagtest.cpp (2 hunks)
- docker/apps/Dockerfile.cloud-server (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug-clang (1 hunks)
- include/oc_rep.h (1 hunks)
- messaging/coap/observe.c (1 hunks)
- port/android/Makefile (1 hunks)
- port/arduino/adapter/Makefile (1 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/linux/Makefile (1 hunks)
- port/openthread/Makefile (1 hunks)
- port/unittest/clocktest.cpp (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (2 hunks)
- security/oc_acl.c (2 hunks)
- security/oc_acl_internal.h (2 hunks)
- security/oc_acl_util.c (1 hunks)
- security/oc_acl_util_internal.h (1 hunks)
- security/unittest/acltest.cpp (1 hunks)
- security/unittest/keypairtest.cpp (1 hunks)
- util/oc_atomic.h (2 hunks)
- util/unittest/memblocktest.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- api/unittest/etagtest.cpp
- docker/apps/Dockerfile.cloud-server
- docker/apps/Dockerfile.cloud-server-debug
- docker/apps/Dockerfile.cloud-server-debug-clang
- include/oc_rep.h
- port/unittest/clocktest.cpp
- security/unittest/keypairtest.cpp
- util/oc_atomic.h
- util/unittest/memblocktest.cpp
Additional comments: 21
security/oc_acl_util_internal.h (1)
- 32-33: The addition of the
oc_sec_check_aclfunction declaration enhances ACL handling by providing a mechanism to check access permissions for a given method, resource, and endpoint. This is a positive change towards improving security.port/openthread/Makefile (1)
- 60-60: The inclusion of
oc_acl_util.cin theSRC_COMMONlist when theSECUREflag is enabled is a logical step towards enhancing security features. This ensures that ACL utility functionalities are available for security-related operations.security/oc_acl_internal.h (1)
- 74-77: The addition of the
oc_sec_ace_find_resourcefunction enhances ACL handling by providing a targeted mechanism to find resources within an ACE. This is a positive change towards improving security and resource management.port/arduino/adapter/Makefile (1)
- 42-42: The inclusion of
oc_acl_util.cin the security sources list when theSECUREflag is enabled is a logical step towards enhancing security features. This ensures that ACL utility functionalities are available for security-related operations.security/unittest/acltest.cpp (1)
- 30-30: The inclusion of
security/oc_acl_util_internal.hin the unit tests is a positive step towards enhancing testing coverage for ACL-related functionalities. This aligns with the PR's objectives of improving security and robustness.port/esp32/main/CMakeLists.txt (1)
- 188-188: The inclusion of
oc_acl_util.cin the sources list whenCONFIG_SECUREis enabled is a logical step towards enhancing security features. This ensures that ACL utility functionalities are available for security-related operations.port/android/Makefile (1)
- 272-272: The addition of
oc_acl_util.cto the security-related source files in the Makefile is noted and appears correctly implemented. Ensure thatoc_acl_util.cexists and contains no errors to prevent build issues.security/oc_acl_util.c (1)
- 1-492: The new file
oc_acl_util.cintroduces several functions for ACL management and checking. It's well-structured and follows the project's coding standards. However, ensure thorough testing, especially for the new logic introduced in functions likeoc_sec_check_acl, to validate their correctness and robustness in various scenarios.security/oc_acl.c (1)
- 109-109: The function
oc_sec_ace_find_resourcehas been modified or added. It's crucial to ensure that the logic for finding a resource within an ACE (Access Control Entry) correctly handles thehrefandwildcardparameters. This function appears to perform a detailed comparison, including handling cases where thehrefmight not start with a/character and wildcard permissions. Ensure that all edge cases are covered, especially with wildcard permissions and partial matches ofhref.port/windows/vs2015/IoTivity-lite.vcxproj (2)
- 275-275: The inclusion of
oc_acl_util_internal.hin the project file ensures that the security-related functionality is accessible during the build process. This is a positive change, enhancing the project's security capabilities.- 456-456: The addition of
oc_acl_util.cto the compilation sources is crucial for implementing the security functionalities defined in the corresponding header file. This change is necessary for the completeness and functionality of the security features.port/windows/vs2015/IoTivity-lite.vcxproj.filters (2)
- 362-364: The addition of
oc_acl_util.cto the Security filter is appropriate, as it aligns with the project's organizational structure, grouping related security functionalities together.- 730-732: Similarly, the inclusion of
oc_acl_util_internal.hin the Security filter is consistent with the project's approach to organizing internal headers related to security features.port/linux/Makefile (1)
- 294-294: The addition of
oc_acl_util.cto the list of security-related source files when theSECUREflag is enabled appears correct. Ensure thatoc_acl_util.cintegrates well with the existing security features and does not introduce any build issues or unhandled dependencies.api/oc_collection.c (1)
- 43-43: The addition of the include directive for
"security/oc_acl_util_internal.h"within the#ifdef OC_SECURITYblock seems appropriate, assuming there's a direct or indirect dependency on this header when the security feature is enabled. Ensure that the contents of this header are indeed utilized within this file or through included headers to justify its inclusion.api/oc_discovery.c (4)
- 51-51: The inclusion of
"security/oc_acl_util_internal.h"suggests a dependency on ACL utilities within the discovery component. Please verify the necessity of this include directive, ensuring it's used within the file or indirectly influences the functionality in a manner not visible in the provided context.- 48-54: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1250]
Consider reviewing the discovery process for potential optimizations, particularly in reducing iterations over endpoints and resources, to improve response times and overall performance.
- 48-54: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1250]
To enhance maintainability and readability, consider modularizing larger functions and further separating concerns within the code. This approach could simplify future modifications and improve the overall structure.
- 48-54: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1250]
Improving documentation and commenting, especially for complex sections of the code, could enhance understanding and adherence to best practices. Consider adding more detailed comments explaining the logic and decisions behind critical sections.
messaging/coap/observe.c (1)
- 79-79: The inclusion of
"security/oc_acl_util_internal.h"suggests enhancements or checks related to ACLs within the observe functionality. Ensure that the functions or definitions provided by this header are indeed utilized within this file to justify its inclusion.api/oc_ri.c (1)
- 86-86: Including
"security/oc_acl_util_internal.h"is noted. Ensure that functions or definitions from this header are indeed utilized withinapi/oc_ri.c. Unnecessary includes should be avoided to keep the codebase clean and maintainable.
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
b2470e9 to
1f4df38
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (25)
- api/oc_collection.c (1 hunks)
- api/oc_discovery.c (1 hunks)
- api/oc_ri.c (1 hunks)
- api/unittest/etagtest.cpp (2 hunks)
- docker/apps/Dockerfile.cloud-server (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug-clang (1 hunks)
- include/oc_rep.h (1 hunks)
- messaging/coap/observe.c (1 hunks)
- port/android/Makefile (1 hunks)
- port/arduino/adapter/Makefile (1 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/linux/Makefile (1 hunks)
- port/openthread/Makefile (1 hunks)
- port/unittest/clocktest.cpp (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (2 hunks)
- security/oc_acl.c (2 hunks)
- security/oc_acl_internal.h (2 hunks)
- security/oc_acl_util.c (1 hunks)
- security/oc_acl_util_internal.h (1 hunks)
- security/unittest/acltest.cpp (1 hunks)
- security/unittest/keypairtest.cpp (1 hunks)
- util/oc_atomic.h (2 hunks)
- util/unittest/memblocktest.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (24)
- api/oc_collection.c
- api/oc_discovery.c
- api/oc_ri.c
- api/unittest/etagtest.cpp
- docker/apps/Dockerfile.cloud-server
- docker/apps/Dockerfile.cloud-server-debug
- docker/apps/Dockerfile.cloud-server-debug-clang
- include/oc_rep.h
- messaging/coap/observe.c
- port/android/Makefile
- port/arduino/adapter/Makefile
- port/esp32/main/CMakeLists.txt
- port/linux/Makefile
- port/openthread/Makefile
- port/unittest/clocktest.cpp
- port/windows/vs2015/IoTivity-lite.vcxproj
- port/windows/vs2015/IoTivity-lite.vcxproj.filters
- security/oc_acl_internal.h
- security/oc_acl_util.c
- security/oc_acl_util_internal.h
- security/unittest/acltest.cpp
- security/unittest/keypairtest.cpp
- util/oc_atomic.h
- util/unittest/memblocktest.cpp
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- security/oc_acl.c (3 hunks)
- security/oc_acl_util.c (1 hunks)
- security/unittest/acltest.cpp (10 hunks)
Files skipped from review as they are similar to previous changes (2)
- security/oc_acl.c
- security/oc_acl_util.c
Additional comments: 13
security/unittest/acltest.cpp (13)
- 30-36: The addition of include directives for internal and test-related headers enhances the test file's capability to perform more comprehensive tests by providing access to necessary internal functionalities and mock objects. This change aligns with the PR's objective to enhance project reliability through unit testing.
- 63-68: The initialization in the
SetUpmethod has been expanded to include the creation of security servers withoc_sec_svr_create(). This is a crucial step for setting up a proper testing environment for ACL tests, ensuring that the security components are initialized correctly before each test case runs. It's important to ensure that all resources initialized here are properly cleaned up in theTearDownmethod to prevent resource leaks.- 74-80: The cleanup in the
TearDownmethod now includesoc_sec_svr_free()andoc_connectivity_shutdown(kDeviceID), which are essential for releasing resources allocated during the test setup. This ensures that each test case starts with a clean state and that there are no resource leaks. It's good practice to mirror the initialization steps inSetUpwith corresponding cleanup steps inTearDown.- 131-170: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [102-180]
The test cases added for ACL functionalities, such as adding bootstrap ACLs, clearing ACLs, and verifying ACL checks under various conditions, significantly contribute to the project's testing framework. These tests cover crucial aspects of ACL management and validation, aligning with the PR's objective to enhance code reliability through thorough testing. It's important to ensure that these tests cover a wide range of scenarios, including edge cases, to fully validate the ACL functionalities.
- 196-202: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [199-215]
The test case
oc_sec_check_acl_in_RFOTMeffectively tests ACL checks in the RFOTM (Request For Ownership Transfer Mode) state, covering scenarios with different permissions and resource access settings. This test case is particularly important for validating security behavior during the ownership transfer process. It's commendable that the test covers both positive and negative scenarios, ensuring a comprehensive validation of ACL checks in RFOTM.
- 232-251: The test case
oc_sec_check_acl_FailInsecureDOCis designed to verify that ACL checks fail for insecure Device Onboarding Client (DOC) connections in the RFOTM state. This test ensures that only secure connections are allowed for sensitive operations, aligning with security best practices. It's crucial to validate the setup and teardown of TLS peers within the test to ensure that the test environment accurately reflects the intended security conditions.- 255-268: The test case
oc_sec_check_acl_AccessInRFOTMvalidates ACL checks for resources in the RFOTM state, focusing on the impact of theSetAccessInRFOTMfunction. This test case contributes to the thorough testing of ACL behavior in different device states, which is essential for ensuring the security and reliability of the system. It's important to cover various combinations of permissions to fully assess the ACL checks' behavior in RFOTM.- 272-289: The test case
oc_sec_check_acl_FailNCRInNonRFNOPchecks that Non-Configurable Resources (NCRs) cannot be accessed in non-RFNOP (Ready For Normal Operation) states. This test is crucial for validating the system's behavior in ensuring that sensitive resources are protected in states that are not ready for normal operation. It's important to test this behavior across all relevant device states to ensure comprehensive coverage.- 291-303: The test case
oc_sec_check_acl_FailInsecureAccessToVerticalResourceensures that insecure access to vertical resources is correctly blocked in the RFNOP state. This test aligns with the objective of securing vertical resources by enforcing strict access controls. It's essential to validate that the test accurately identifies vertical resources and applies the correct ACL checks.- 305-328: The test case
oc_sec_check_acl_DOCAccessToDCRvalidates that Device Onboarding Client (DOC) access to Device Configuration Resources (DCRs) is correctly handled in the RFOTM state. This test is important for ensuring that the system correctly manages access controls during the device onboarding process. It's crucial to ensure that the test setup accurately reflects the security conditions expected during DOC access to DCRs.- 330-345: The test case
oc_sec_check_acl_GETinRFOTMverifies that core resources are accessible via GET requests in the RFOTM state. This test is essential for ensuring that the system behaves as expected during the ownership transfer process, allowing access to necessary resources. It's important to cover all core resources to ensure comprehensive testing of ACL checks in RFOTM.- 347-361: The test case
oc_sec_check_acl_PriorToDOCAccessToDoxmInRFOTMensures that access to thedoxmresource is correctly handled prior to DOC access in the RFOTM state. This test is crucial for validating the system's behavior in managing access controls during sensitive stages of the device lifecycle. It's important to test various methods to fully assess access controls for thedoxmresource.- 363-377: The test case
oc_sec_check_acl_PriorToDOCGetAccessToPstatInRFOTMfocuses on validating GET access to thepstatresource prior to DOC access in the RFOTM state. This test is important for ensuring that the system correctly manages access controls for critical resources during the device onboarding process. It's commendable that the test specifically targets GET access while also checking that other methods are correctly blocked.
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
d08f024 to
ff8b0d1
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- security/oc_acl.c (3 hunks)
- security/oc_acl_util.c (1 hunks)
- security/unittest/acltest.cpp (10 hunks)
Files skipped from review as they are similar to previous changes (3)
- security/oc_acl.c
- security/oc_acl_util.c
- security/unittest/acltest.cpp
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
ff8b0d1 to
e98425f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- security/oc_acl.c (3 hunks)
- security/oc_acl_util.c (1 hunks)
- security/unittest/acltest.cpp (10 hunks)
Files skipped from review as they are similar to previous changes (3)
- security/oc_acl.c
- security/oc_acl_util.c
- security/unittest/acltest.cpp
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
e98425f to
1aea124
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (36)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/oc_collection.c (1 hunks)
- api/oc_core_res.c (1 hunks)
- api/oc_discovery.c (1 hunks)
- api/oc_etag.c (1 hunks)
- api/oc_ri.c (1 hunks)
- api/unittest/etagtest.cpp (2 hunks)
- docker/apps/Dockerfile.cloud-server (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug (1 hunks)
- docker/apps/Dockerfile.cloud-server-debug-clang (1 hunks)
- include/oc_rep.h (1 hunks)
- messaging/coap/observe.c (1 hunks)
- port/android/Makefile (1 hunks)
- port/arduino/adapter/Makefile (1 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/linux/Makefile (1 hunks)
- port/openthread/Makefile (1 hunks)
- port/unittest/clocktest.cpp (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (2 hunks)
- security/oc_acl.c (3 hunks)
- security/oc_acl_internal.h (2 hunks)
- security/oc_acl_util.c (1 hunks)
- security/oc_acl_util_internal.h (1 hunks)
- security/oc_obt.c (1 hunks)
- security/oc_obt_otm_cert.c (4 hunks)
- security/oc_obt_otm_justworks.c (4 hunks)
- security/oc_obt_otm_randompin.c (4 hunks)
- security/oc_pstat.c (5 hunks)
- security/oc_pstat_internal.h (5 hunks)
- security/oc_svr.c (1 hunks)
- security/unittest/acltest.cpp (10 hunks)
- security/unittest/keypairtest.cpp (1 hunks)
- security/unittest/pstattest.cpp (3 hunks)
- util/oc_atomic.h (2 hunks)
- util/unittest/memblocktest.cpp (1 hunks)
Files not reviewed due to errors (2)
- (no review received)
- (no review received)
Files skipped from review due to trivial changes (1)
- security/oc_obt_otm_justworks.c
Additional comments: 81
docker/apps/Dockerfile.cloud-server-debug (1)
- 17-17: Using
WORKDIRbefore cloninglibfaketimeis a good practice as it avoids the need forcdcommands and makes the Dockerfile more readable.security/oc_acl_util_internal.h (1)
- 1-39: The header file
oc_acl_util_internal.his well-structured and follows best practices for C header files, including include guards and conditional C++ linkage. The function declaration foroc_sec_check_aclis clear and appropriately documented.docker/apps/Dockerfile.cloud-server (1)
- 1-1: Switching to a specific version of Alpine (
alpine:3.19) instead of usinglatestis a good practice for ensuring consistent and predictable builds.docker/apps/Dockerfile.cloud-server-debug-clang (2)
- 6-11: Refactoring the package installation to separate the installation of
clang-15and setting it as the default compiler is a clear and efficient approach. It improves readability and maintainability of the Dockerfile.- 23-26: Specifying
CC=clangfor themakecommand when installinglibfaketimeensures that the correct compiler is used. This is a good practice when multiple compilers are available in the environment.security/oc_svr.c (1)
- 51-51: Replacing direct resource creation with a call to
oc_sec_pstat_create_resourcecentralizes the logic for creating theOCF_SEC_PSTATresource, which can improve maintainability and consistency across the codebase.port/openthread/Makefile (1)
- 60-60: Adding
oc_acl_util.cto theSRC_COMMONlist when theSECUREflag is set to 1 ensures that ACL utility functions are available for secure builds. This change aligns with the project's approach to conditional compilation based on security requirements.security/oc_acl_internal.h (1)
- 74-78: The addition of
oc_sec_ace_find_resourcefunction enhances the ACL module by providing a way to find resources within an ACE. This function could be crucial for fine-grained access control checks and should be thoroughly tested to ensure it behaves as expected.Verification successful
The function
oc_sec_ace_find_resourceis correctly integrated and used across the project, specifically within the ACL module, to enhance access control checks by providing a mechanism to find resources within an ACE. The usage patterns observed insecurity/oc_acl_util.candsecurity/oc_acl.cconfirm its role and importance in the project's ACL handling logic. This aligns with the initial review comment, affirming the function's addition and its expected behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that oc_sec_ace_find_resource is used correctly across the project rg "oc_sec_ace_find_resource" --type cLength of output: 500
port/arduino/adapter/Makefile (1)
- 42-42: Including
oc_acl_util.cin the security source files for the Arduino adapter ensures that ACL utility functions are compiled into the security-enabled builds. This is a necessary step for supporting advanced security features on the Arduino platform.util/unittest/memblocktest.cpp (2)
- 1-124: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [163-164]
In the
cloud_publish_resourcesfunction, the condition to check if the device is in theOC_DOS_RFNOPstate has been updated to useoc_device_is_in_dos_stateinstead ofoc_sec_pstat_is_in_dos_state. This change aligns with the PR objectives to replace the function calls as mentioned in the AI-generated summary. This update is consistent and correctly implements the intended functionality.
- 1-124: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [256-257]
Similarly, in the
cloud_delete_resourcesfunction, the condition to check if the device is in theOC_DOS_RFNOPstate has been updated to useoc_device_is_in_dos_stateinstead ofoc_sec_pstat_is_in_dos_state. This change is in line with the PR objectives and correctly applies the intended update.security/oc_pstat_internal.h (5)
- 22-22: The inclusion of
oc_helpers_internal.his a new addition to this file. This change is likely made to use utility functions or definitions provided byoc_helpers_internal.hwithinoc_pstat_internal.h. Ensure that the functions or definitions fromoc_helpers_internal.hare indeed used in this file to justify the inclusion.- 33-34: The definitions for
OCF_SEC_PSTAT_URIandOCF_SEC_PSTAT_RTare added to standardize the URI and resource type for the pstat resource. This is a good practice as it avoids hardcoding strings in multiple places, making the code more maintainable and less error-prone.- 37-37: The function
oc_sec_pstat_create_resourceis added to create a pstat resource for a given device. This addition seems to be part of the reorganization of the access control logic as mentioned in the PR objectives. Ensure that this function is implemented with proper error handling and security checks, especially considering its role in security configuration.- 41-41: The function
oc_sec_is_pstat_resource_urichecks if a given URI matches the pstat resource URI. This utility function can simplify URI matching logic elsewhere in the codebase, contributing to cleaner and more readable code. Verify that it's used effectively where URI checks for the pstat resource are needed.Verification successful
The function
oc_sec_is_pstat_resource_uriis effectively used within the codebase, specifically inoc_acl_util.c, to simplify URI matching logic for the pstat resource, as intended. This contributes to cleaner and more readable code where URI checks for the pstat resource are needed.* 124-124: The function `oc_sec_pstat_is_in_dos_state` checks if the onboarding state property is in one of the DOS states. This function, along with `oc_device_is_in_dos_state`, provides a clearer and more direct way to query the device state, which is crucial for making security-related decisions. Ensure that these functions are used consistently across the codebase for checking DOS states.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of oc_sec_is_pstat_resource_uri to ensure it's effectively used grep -r "oc_sec_is_pstat_resource_uri" security/Length of output: 392
Verification successful
The provided grep command and its output show that
oc_sec_pstat_is_in_dos_stateandoc_device_is_in_dos_stateare used within thesecurity/directory, indicating a consistent usage pattern in this part of the codebase. To fully verify the comment regarding their consistent usage across the entire codebase, it would be beneficial to extend the search beyond thesecurity/directory. This will ensure that these functions are indeed used consistently everywhere they are needed for making security-related decisions.
The extended search across the entire codebase has revealed that
oc_sec_pstat_is_in_dos_stateandoc_device_is_in_dos_stateare consistently used not only within thesecurity/directory but also in other parts of the codebase, such as./api/cloud/oc_cloud_rd.cand./api/oc_etag.c. This indicates a comprehensive and logical application of these functions for making security-related decisions, aligning with the guidance provided in the review comment. Therefore, the usage of these functions across the codebase supports the comment's assertion regarding their importance in querying the device state for security purposes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure consistent usage of oc_sec_pstat_is_in_dos_state and oc_device_is_in_dos_state across the codebase grep -rE "oc_sec_pstat_is_in_dos_state|oc_device_is_in_dos_state" security/Length of output: 640
Script:
#!/bin/bash # Extend the search for oc_sec_pstat_is_in_dos_state and oc_device_is_in_dos_state across the entire codebase grep -rE "oc_sec_pstat_is_in_dos_state|oc_device_is_in_dos_state" .Length of output: 884
port/unittest/clocktest.cpp (1)
- 72-72: The change to cast the result of the time difference calculation to a
longtype in theoc_clock_waittest function is appropriate. This ensures that the type of the variablesecondsmatches the expected type for the assertion that follows. This change improves type safety and clarity in the test.security/unittest/pstattest.cpp (3)
- 181-181: Replacing the hardcoded URI
"/oic/sec/pstat"with the constantOCF_SEC_PSTAT_URIin thePostRequest_FailMethodNotAuthorizedtest case is a good practice. It ensures consistency and makes the code easier to maintain.- 197-197: Similarly, using the
OCF_SEC_PSTAT_URIconstant in thePutRequest_Failtest case instead of a hardcoded URI string is a positive change. It enhances code readability and maintainability.- 211-211: The use of
OCF_SEC_PSTAT_URIin theDeleteRequest_Failtest case, replacing the hardcoded URI, follows the same pattern of improving code maintainability and consistency. This is a welcomed change.util/oc_atomic.h (2)
- 41-44: Converting macro definitions to typedefs for atomic integer types (
OC_ATOMIC_INT8_T,OC_ATOMIC_UINT8_T,OC_ATOMIC_INT32_T,OC_ATOMIC_UINT32_T) is a positive change. It enhances type safety and readability of the code. This change aligns with best practices for defining atomic types.- 83-86: Similarly, for Windows platforms, converting macro definitions to typedefs for atomic integer types is a good practice. It ensures consistency across different platforms and improves the readability and maintainability of the code.
api/cloud/oc_cloud_rd.c (2)
- 163-164: In the
cloud_publish_resourcesfunction, the condition to check if the device is in theOC_DOS_RFNOPstate has been updated to useoc_device_is_in_dos_stateinstead ofoc_sec_pstat_is_in_dos_state. This change aligns with the PR objectives to replace the function calls as mentioned in the AI-generated summary. This update is consistent and correctly implements the intended functionality.- 256-257: Similarly, in the
cloud_delete_resourcesfunction, the condition to check if the device is in theOC_DOS_RFNOPstate has been updated to useoc_device_is_in_dos_stateinstead ofoc_sec_pstat_is_in_dos_state. This change is in line with the PR objectives and correctly applies the intended update.port/esp32/main/CMakeLists.txt (1)
- 189-189: The addition of
${CMAKE_CURRENT_SOURCE_DIR}/../../../security/oc_acl_util.cunder theCONFIG_SECUREflag is correctly implemented. Ensure thatCONFIG_SECUREis consistently used and documented across the project. Also, verify ifoc_acl_util.chas any other dependencies or requires additional configurations.security/unittest/acltest.cpp (16)
- 30-37: Added includes for internal security modules are appropriate for the expanded test coverage. Ensure these modules are only used for testing purposes and do not introduce unnecessary dependencies in the production code.
- 61-103: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [47-72]
The modifications in the
SetUpmethod, including the initialization of security modules and setting the log level to debug, are suitable for the intended test setup. However, consider resetting the log level back to its original state after tests to avoid affecting other tests or the production environment.
- 77-83: In
TearDown, resetting the log level to INFO is a good practice. Ensure that all resources initialized inSetUpare correctly freed or reset here to prevent resource leaks or interference with other tests.- 90-100: The method
getSVRsis a useful addition for retrieving Secure Virtual Resources (SVRs) for testing. Ensure that the range of resources it iterates over (OCF_SEC_DOXMtoOCF_D) correctly encompasses all SVRs intended for testing.- 117-120: The test
oc_sec_acl_add_bootstrap_aclcorrectly checks the addition of a bootstrap ACL. Ensure that edge cases, such as adding a bootstrap ACL when one already exists, are also tested.- 146-185: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [130-154]
In the test
oc_sec_acl_clear, the use ofoc_sec_ace_update_resto set up test conditions is appropriate. However, consider adding comments to explain the purpose of each block within the test for clarity, especially for complex test setups.
- 210-216: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [213-242]
The test
oc_sec_check_acl_in_RFOTMintroduces testing for security checks in different device states, which is crucial for ensuring the correct behavior of ACLs. Ensure that all relevant device states are covered and consider adding negative tests for comprehensive coverage.
- 246-265: The test
oc_sec_check_acl_FailInsecureDOCis important for verifying the behavior of ACL checks with insecure device ownership credentials. Ensure that the setup for the TLS peer and endpoint is correctly mimicking the intended insecure conditions.Verification successful
The provided script outputs and context give us insight into how the
oc_tls_add_or_get_peerandoc_sec_check_aclfunctions operate, as well as the conditions under which they are called within the testoc_sec_check_acl_FailInsecureDOC.From the
oc_tls.cfile, we see thatoc_tls_add_or_get_peeradds a new TLS peer based on the provided endpoint and role, with an emphasis on whether the peer was newly created or already existed. This function is crucial for establishing a secure or insecure connection, depending on the test scenario.The
oc_sec_check_aclfunction, as seen inoc_acl_util.c, performs access control checks based on the method, resource, and endpoint involved in the request. It includes checks for various device ownership states (DOS), whether the request is over a secured channel, and if the resource is a Device Configuration Resource (DCR), a Secure Virtual Resource (SVR), or a vertical resource. The function's logic is comprehensive, covering different scenarios including access during the RFOTM (Requesting Full Ownership Transfer Method) state and prior to Device Onboarding Completion (DOC).The test
oc_sec_check_acl_FailInsecureDOCspecifically sets up a scenario where a TLS peer is added with the intention of mimicking insecure conditions by asserting that the endpoint is not secured (ASSERT_EQ(0, ep.flags & SECURED)). This setup, followed by the call tooc_sec_check_aclwith the expectation ofEXPECT_FALSE, aims to verify that access control correctly identifies and blocks insecure access attempts under specific conditions (in this case, likely during the RFOTM state or prior to DOC).Given the detailed implementation of both
oc_tls_add_or_get_peerandoc_sec_check_acl, and the specific assertions and expectations within the testoc_sec_check_acl_FailInsecureDOC, it can be inferred that the test is designed to accurately mimic and verify the behavior of ACL checks under insecure conditions as intended.Therefore, based on the analysis of the provided context and the script outputs, the review comment regarding the verification of the test setup for mimicking insecure conditions in
oc_sec_check_acl_FailInsecureDOCis consistent with the codebase and its intended functionality.* 267-282: The test `oc_sec_check_acl_AccessInRFOTM` correctly verifies ACL access in the RFOTM state. Ensure that the test setup accurately reflects the RFOTM state and that the resource setup is consistent with the intended test conditions. * 286-303: The test `oc_sec_check_acl_FailNCRInNonRFNOP` is crucial for ensuring that non-configured resources (NCRs) are not accessible in non-RFNOP states. Consider adding a brief comment explaining the significance of each state tested to enhance readability and maintainability. * 305-317: The test `oc_sec_check_acl_FailInsecureAccessToVerticalResource` ensures that insecure access to vertical resources is correctly handled. Verify that the definition of "vertical resource" in this context is consistent with the project's security model.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the setup for TLS peer and endpoint in oc_sec_check_acl_FailInsecureDOC correctly mimics insecure conditions. # This script is a placeholder for the actual verification logic, which would depend on the project's documentation or source code structure. echo "Verify the TLS peer and endpoint setup in oc_sec_check_acl_FailInsecureDOC."Length of output: 160
Script:
#!/bin/bash # Search for the implementation of oc_tls_add_or_get_peer and oc_sec_check_acl to understand their handling of security conditions. rg "oc_tls_add_or_get_peer" -A 30 rg "oc_sec_check_acl" -A 30 # Additionally, look for any documentation or comments that define "insecure conditions". rg "insecure conditions" -C 5Length of output: 55854
Verification successful
The definition of "vertical resource" within the project's security model is consistent across various parts of the codebase. Vertical resources are identified as custom resources specific to a device, subject to strict access control measures, and play a significant role in the project's ecosystem, including cloud registration and discovery. This understanding aligns with the project's security model as inferred from the code snippets provided in the shell script output. Therefore, the review comment is verified as the definition of "vertical resource" is consistent with the project's security model.
* 319-342: The test `oc_sec_check_acl_DOCAccessToDCR` effectively tests DOC (Device Ownership Credential) access to DCR (Device Configuration Resources). Ensure that the setup for the TLS peer and endpoint accurately represents secure DOC conditions. * 344-359: The test `oc_sec_check_acl_GETinRFOTM` is important for verifying GET access to core resources in RFOTM. Ensure that the selection of resources (`OCF_P`, `OCF_D`, `OCF_RES`) is comprehensive and relevant to the RFOTM state. * 361-375: The test `oc_sec_check_acl_PriorToDOCAccessToDoxmInRFOTM` correctly verifies access to the `doxm` resource prior to DOC in RFOTM. This is crucial for the device ownership transfer process. Ensure that the test accurately reflects the intended conditions for DOC access. * 377-392: The test `oc_sec_check_acl_PriorToDOCGetAccessToPstatInRFOTM` effectively tests GET access to `pstat` prior to DOC in RFOTM. Consider adding tests for other methods (POST, PUT, DELETE) to ensure comprehensive coverage of access controls. * 394-415: The test `oc_sec_check_acl_FailInsecureAccessToSecurityVerticalResource` ensures that insecure access to security vertical resources is correctly restricted. This is a critical aspect of the security model. Verify that all relevant SVRs and device states are covered in the test.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the definition of "vertical resource" in the context of the project's security model. # This script is a placeholder for the actual verification logic, which would depend on the project's documentation or source code structure. echo "Verify the definition of 'vertical resource' in oc_sec_check_acl_FailInsecureAccessToVerticalResource."Length of output: 220
Script:
#!/bin/bash # Search for mentions of "vertical resource" within the codebase to understand its definition and role in the security model. rg "vertical resource" --context 5 # Additionally, search for documentation or comments that might explain the concept further. rg "vertical resource" -tmdLength of output: 5318
Verification successful
The provided script output contains extensive references to the handling of SVRs (Security Vertical Resources) and device states within the codebase. The references span across various files and contexts, including unit tests, security implementations, cloud interactions, and more. This information is crucial for verifying the coverage of SVRs and device states in the test
oc_sec_check_acl_FailInsecureAccessToSecurityVerticalResource.Analysis of Script Output:
SVRs Identification and Handling:
- The function
oc_core_is_SVRis used across multiple files to identify SVRs. This function checks if a given resource is an SVR based on the device ID.- The unit test file
unittest/acltest.cppspecifically tests the handling of SVRs in various device states, including the test in question.Device States Coverage:
- The device states
OC_DOS_RFPRO,OC_DOS_RFNOP, andOC_DOS_SRESETare explicitly tested in the unit testoc_sec_check_acl_FailInsecureAccessToSecurityVerticalResource, as seen in the provided code snippet.- The script output also shows various parts of the codebase reacting to different device states, indicating a comprehensive handling of device states across the project.
Relevance to Security Model:
- The handling of SVRs and device states is closely tied to the security model of the system. The tests and code snippets ensure that SVRs are appropriately restricted based on the device's state, aligning with security requirements.
Conclusion:
The script output provides evidence that the test
oc_sec_check_acl_FailInsecureAccessToSecurityVerticalResourceand related code adequately cover the handling of SVRs and relevant device states (OC_DOS_RFPRO,OC_DOS_RFNOP, andOC_DOS_SRESET). This coverage is crucial for maintaining the security model's integrity, ensuring that SVRs are protected against unauthorized access in sensitive device states.Given the detailed references to SVRs and device states handling in the provided script output, it can be concluded that the test and related codebase sections comprehensively cover the necessary aspects of SVRs and device states as per the security model requirements.
**Final Tag: **
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all relevant SVRs and device states are covered in oc_sec_check_acl_FailInsecureAccessToSecurityVerticalResource. # This script is a placeholder for the actual verification logic, which would depend on the project's documentation or source code structure. echo "Verify SVRs and device states coverage in oc_sec_check_acl_FailInsecureAccessToSecurityVerticalResource."Length of output: 218
Script:
#!/bin/bash # Search for definitions or listings of SVRs in the codebase. rg "oc_core_is_SVR" --context 5 # Search for definitions or usages of device states to ensure coverage. rg "OC_DOS_" --context 5Length of output: 78682
port/android/Makefile (1)
- 272-272: The addition of
oc_acl_util.cto theSRClist under theSECUREcondition seems appropriate, assumingoc_acl_util.cis indeed part of the security features. Please ensure that its inclusion is necessary and correct for builds whereSECUREis enabled.security/unittest/keypairtest.cpp (1)
- 279-290: The modifications to
EXPECTstatements by removing concatenated error messages simplify the code. However, please consider the impact on the clarity of test failures. Ensure that test names and any additional context or comments are sufficient to understand failures without these detailed messages.security/oc_obt_otm_cert.c (2)
- 79-79: The replacement of the hardcoded URI with
OCF_SEC_PSTAT_URImacro enhances maintainability and consistency. Good job on this improvement.- 247-247: The use of
OCF_SEC_PSTAT_URImacro here is consistent with the previous change and continues to improve code maintainability. Well done.security/oc_acl_util.c (1)
- 1-1: Updating the copyright information to include "plgd.dev s.r.o." for the year 2024 is appropriate and keeps the file's metadata current.
security/oc_obt_otm_randompin.c (3)
- 79-79: Using
OCF_SEC_PSTAT_URIconstant instead of a hardcoded URI string improves maintainability and consistency. Good change.- 247-247: This change aligns with the previous one, consistently using
OCF_SEC_PSTAT_URIfor URI references. Well done.- 425-425: Consistent use of
OCF_SEC_PSTAT_URIacross the file is a good practice for maintainability. Approved.api/oc_etag.c (1)
- 444-444: Replacing
oc_sec_pstat_is_in_dos_statewithoc_device_is_in_dos_statecould streamline device state checking. Ensure that the new function accurately reflects the intended state checks and does not introduce regressions.Verification successful
The provided output from the
oc_pstat.cfile reveals the implementation details of theoc_device_is_in_dos_statefunction. This function checks if a device is in a specific Device Onboarding State (DOS) by comparing the device's current state against a provided mask. The function utilizes theoc_sec_pstat_is_in_dos_statefunction, which operates on aoc_sec_pstat_tstructure representing the persistent state of security resources for a device. Theoc_device_is_in_dos_statefunction effectively abstracts the process of checking the device's state, making it easier to use across different parts of the codebase, such as in cloud and ETag functionalities.Given the context and the implementation details, it's clear that the replacement of
oc_sec_pstat_is_in_dos_statewithoc_device_is_in_dos_statein theoc_etag.cfile is intended to streamline the process of checking the device's state. This change leverages a more direct and possibly more readable approach to perform the state check, which could improve code maintainability and readability without altering the underlying logic of state checking.Considering the usage of
oc_device_is_in_dos_statein multiple parts of the codebase and its implementation, it can be inferred that the function accurately reflects the intended state checks. Therefore, the change in theoc_etag.cfile is consistent with the goal of streamlining device state checking, and there's no indication that it introduces regressions or deviates from the intended functionality.Based on the analysis, the review comment is verified as the change seems appropriate for the intended purpose, and the function
oc_device_is_in_dos_stateis implemented in a way that supports the described functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that oc_device_is_in_dos_state correctly handles device state checks grep -R "oc_device_is_in_dos_state" .Length of output: 456
Script:
#!/bin/bash # View the implementation of oc_device_is_in_dos_state in oc_pstat.c cat ./security/oc_pstat.cLength of output: 21221
security/oc_pstat.c (6)
- 22-25: The addition of
#include "api/oc_resource_internal.h"is necessary for the newly introduced functions that deal with resource URIs. This change seems appropriate and necessary for the functionality being introduced.- 449-452: The modification of
oc_sec_pstat_is_in_dos_stateto take aconst oc_sec_pstat_t *psas input instead of a device index is a good practice. It makes the function more flexible and reusable by directly working with theoc_sec_pstat_tstructure. This change enhances the modularity and maintainability of the code.- 454-457: The addition of
oc_device_is_in_dos_statefunction is a logical extension to the existing functionality, providing a convenient way to check the device state using the device index. This function complements the modifiedoc_sec_pstat_is_in_dos_statefunction and improves the API's usability.- 671-679: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [674-692]
The refactoring of
get_pstatandpost_pstatfunctions intopstat_resource_getandpstat_resource_postis a positive change. It aligns the function names with the naming conventions used throughout the codebase, improving consistency and readability. Additionally, this change makes the purpose of these functions clearer to new readers or contributors to the codebase.
- 806-812: The addition of
oc_sec_pstat_create_resourcefunction is crucial for initializing the/oic/sec/pstatresource. This function encapsulates the resource creation process, making the code more modular and easier to maintain. The use ofOC_IF_RW | OC_IF_BASELINEandOC_DISCOVERABLE | OC_OBSERVABLEflags correctly sets the interface and properties of the pstat resource, aligning with the security requirements.- 815-818: The introduction of
oc_sec_is_pstat_resource_urifunction is a good practice for checking if a given URI corresponds to the pstat resource. This function enhances the code's modularity by abstracting the URI check, making it reusable across different parts of the codebase where such a check is needed.api/oc_core_res.c (1)
- 736-736: The change to use
oc_sec_is_pstat_resource_uriinoc_core_get_resource_type_by_urifor checking if a URI corresponds to the pstat resource is a positive improvement. It leverages the newly introduced function to make the URI check more modular and maintainable. This change ensures that the logic for identifying the pstat resource URI is centralized, reducing the risk of inconsistencies and errors.security/oc_acl.c (6)
- 23-35: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-44]
Consider verifying the upper limit of
oc_core_get_num_devices()to prevent potential excessive memory allocation inoc_sec_acl_init. Additionally, it's crucial to handle the case wherecallocfails to allocate memory, which currently leads to a call tooc_abort("Insufficient memory"). While this may be an intended behavior, ensuring graceful error handling and recovery in low-memory situations could enhance the robustness of the system.
- 23-35: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [70-79]
The
get_new_aceidfunction generates a unique ACE identifier using a loop that could potentially become infinite if all possible IDs are taken. Consider adding a maximum attempt counter to this loop to prevent an infinite loop scenario, ensuring the function exits after a reasonable number of attempts even if a unique ID cannot be found.
- 100-106: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [103-134]
The logic within
oc_sec_ace_find_resourceis sound, effectively handling both explicit resource identifiers and wildcard resources. However, the function's complexity and multiple exit points could benefit from a refactor to improve readability and maintainability. Consider simplifying the logic or breaking it down into smaller, more focused functions.
- 23-35: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [234-345]
The
oc_sec_encode_aclfunction's serialization logic is comprehensive, effectively covering various subject types and resource specifications. However, its length and complexity could impact maintainability. Consider refactoring this function into smaller, more manageable pieces or extracting parts of the logic into separate functions to enhance readability and maintainability.
- 23-35: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1035-1048]
The memory management logic within
oc_sec_acl_freeis correctly implemented, ensuring that allocated ACL resources are properly freed, preventing memory leaks. This function appropriately handles the conditional freeing of the ACL list based onOC_DYNAMIC_ALLOCATION, demonstrating good practice in resource management.
- 23-35: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1050-1057]
The implementation of
oc_sec_acl_defaulteffectively clears ACL entries and resets the owner UUID for a given device, aligning with the intended functionality for initializing or resetting ACL settings to their defaults. This straightforward approach is appropriate for the task at hand.port/windows/vs2015/IoTivity-lite.vcxproj (2)
- 275-275: The addition of
oc_acl_util_internal.hto the project file is noted. Ensure that the file path is consistent with the project's directory structure and that the file is correctly placed in the specified directory.- 456-456: The inclusion of
oc_acl_util.cin the project file is appropriate for integrating its functionalities into the build. As with the header file, please verify that the file path aligns with the project's directory structure and that the file is correctly placed.port/windows/vs2015/IoTivity-lite.vcxproj.filters (2)
- 362-364: The addition of
oc_acl_util.cto the Security filter is correctly done and aligns with the project's structure for organizing security-related files.- 730-732: The inclusion of
oc_acl_util_internal.hin the Security filter is appropriate and follows the conventions for organizing internal security-related header files.api/oc_collection.c (1)
- 43-43: The addition of
#include "security/oc_acl_util_internal.h"within the#ifdef OC_SECURITYblock suggests new dependencies on ACL utility functions. Ensure that this inclusion is necessary and correctly enhances the security features as intended by the PR objectives.api/oc_discovery.c (5)
- 51-51: The inclusion of
"security/oc_acl_util_internal.h"suggests that ACL utility functions or definitions from this header are used withinoc_discovery.c. Ensure that the functions or definitions provided by this header are indeed utilized within this file. If this inclusion is primarily for indirect dependencies, consider documenting this explicitly near the include directive for clarity and maintainability.Verification successful
Given the output, there's no direct evidence of the usage of entities from
"security/oc_acl_util_internal.h"withinoc_discovery.c, based solely on the string "oc_acl_util". However, this does not conclusively prove the header is unnecessary, as it might contain macros, type definitions, or indirectly used entities that don't explicitly include "oc_acl_util" in their names. To further investigate, we should examine the contents of"oc_acl_util_internal.h"for potentially relevant entities (like macros or type definitions) and then search for their usage in"oc_discovery.c".
To conclusively determine the necessity of including
"security/oc_acl_util_internal.h"inoc_discovery.c, we need to search for the usage of the functionoc_sec_check_acland the typesoc_method_t,oc_resource_t, andoc_endpoint_twithinoc_discovery.c. This will help us understand if the inclusion is justified by direct or indirect usage of these entities.
The search results indicate that
oc_discovery.cmakes use of the typesoc_endpoint_tandoc_resource_textensively. These types are related to the functionoc_sec_check_acl, which is defined in the included header"security/oc_acl_util_internal.h". Notably, there is a direct call tooc_sec_check_aclwithinoc_discovery.c, specifically in a context that checks access control lists (ACLs) for resources, which justifies the inclusion of"security/oc_acl_util_internal.h".Given this evidence, the inclusion of
"security/oc_acl_util_internal.h"inoc_discovery.cis justified due to the direct usage of theoc_sec_check_aclfunction and the related typesoc_endpoint_tandoc_resource_t. This confirms the necessity of the include directive for the functionality implemented inoc_discovery.c.* 51-51: Including `"security/oc_acl_util_internal.h"` adds specific ACL utility functions or definitions to this file. It's important to ensure that these utilities are indeed used within `oc_discovery.c`. If the inclusion is for indirect dependencies or macro definitions, consider adding a comment near the include directive to clarify its purpose for future maintainability. * 48-54: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1085-1113]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of entities from oc_acl_util_internal.h within oc_discovery.c rg "oc_acl_util" api/oc_discovery.cLength of output: 78
Script:
#!/bin/bash # Examine the contents of oc_acl_util_internal.h for entities that could be used in oc_discovery.c cat security/oc_acl_util_internal.hLength of output: 1206
Script:
#!/bin/bash # Search for the usage of oc_sec_check_acl and related types in oc_discovery.c rg "oc_sec_check_acl|oc_method_t|oc_resource_t|oc_endpoint_t" api/oc_discovery.cLength of output: 2793
In the
discovery_resource_getfunction, there's a conditional compilation block forOC_SPEC_VER_OICthat handles discovery requests specifically for OIC 1.1.0. Ensure that this version-specific handling is still relevant and correctly implemented. Additionally, verify that the security checks (e.g., forOC_SECURITY) and the logic for determining the response based on the interface mask are correctly applied and cover all necessary cases.Verification successful
The review comment is verified. The version-specific handling for OIC 1.1.0 is correctly implemented and relevant, as evidenced by the presence of code blocks enclosed in
#ifdef OC_SPEC_VER_OIC. Additionally, security checks related to discovery requests are correctly applied throughout the discovery process, as indicated by the usage ofOC_SECURITYin various parts of the code. These findings confirm that thediscovery_resource_getfunction includes appropriate version-specific handling and security checks as described.* 48-54: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1155-1216]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the version-specific handling for OIC 1.1.0 is correctly implemented and relevant. grep -A 10 "OC_SPEC_VER_OIC" api/oc_discovery.c # Check for security checks related to discovery requests. grep -A 20 "OC_SECURITY" api/oc_discovery.cLength of output: 12384
The
discovery_encodefunction handles encoding of discovery responses for different interfaces. It's crucial to ensure that the encoding process is efficient and that the code is maintainable. Specifically, review the handling of theOC_IF_Binterface for batch support and verify that the encoding for each interface type is correctly implemented according to the specifications. Additionally, consider if there are opportunities to refactor the code for better readability and maintainability, especially in handling the different interface types.Consider refactoring the
discovery_encodefunction to improve readability and maintainability. One approach could be to separate the encoding logic for each interface type into separate helper functions. This would make the code easier to understand and maintain, especially as new interface types may be added in the future.
- 48-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1085-1113]
Within the
discovery_resource_getfunction, there's conditional logic forOC_SECURITYthat may affect how discovery requests are handled, especially in secure contexts. It's important to ensure that these security checks are correctly implemented and that they adequately protect against unauthorized access to discovery information. Review the security logic to confirm that it aligns with the project's security policies and best practices.Verification successful
The grep output confirms the implementation of security checks within the
api/oc_discovery.cfile, specifically in relation to handling discovery requests in secure contexts. The file includes conditional compilation blocks forOC_SECURITY, indicating comprehensive security-related logic. This includes filtering resources based on security policies, handling secure batch interface requests, and more specific functionalities like handling secure device UUIDs (SDUUID) and secure discovery interfaces (SDI). These implementations align with the concerns for adequately protecting against unauthorized access to discovery information, confirming that the security logic is correctly implemented and aligns with the project's security policies and best practices.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct implementation of security checks in relation to discovery requests. grep -A 20 "OC_SECURITY" api/oc_discovery.cLength of output: 11098
api/unittest/etagtest.cpp (3)
- 687-687: The change to
isPlatformResourceURIfunction signature fromconst std::string&tostd::string_viewis modern and efficient. Ensure all calls to this function have been updated accordingly.Verification successful
The calls to
isPlatformResourceURIwithinetagtest.cppappear to have been updated to be compatible with the new function signature that usesstd::string_view. There's no evidence of misuse or compatibility issues based on the provided context.* 597-599: The refactoring in `isETagStorageEmpty` to simplify the return statement improves readability. Ensure the simplified logic accurately reflects the intended behavior. * 687-687: Ensure that the updated usage of `isPlatformResourceURI` throughout the codebase correctly handles the new `std::string_view` parameter without introducing lifetime issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `isPlatformResourceURI` to ensure they are compatible with the new signature. rg --type cpp "isPlatformResourceURI"Length of output: 255
messaging/coap/observe.c (2)
- 79-79: Including
"security/oc_acl_util_internal.h"introduces tighter coupling between the CoAP observe functionality and the security layer. Ensure that this dependency is necessary and justified, considering the potential impact on modularity and maintainability.- 79-79: The addition of
"security/oc_acl_util_internal.h"suggests enhancements or dependencies related to access control. Verify that the newly introduced functions or data structures from this header are properly utilized, especially in terms of security checks or permissions validation.api/oc_ri.c (2)
- 86-86: The inclusion of
"security/oc_acl_util_internal.h"is noted. Ensure that functions or definitions from this header are indeed utilized withinapi/oc_ri.c. If not, consider removing this include directive to avoid unnecessary dependencies.- 86-86: Given the addition of
"security/oc_acl_util_internal.h", verify that functions or definitions from this header are indeed utilized withinapi/oc_ri.c. If there are no usages found, consider removing this include directive to streamline dependencies and reduce potential for unused code inclusion.security/oc_obt.c (1)
- 874-874: The update to use
OCF_SEC_PSTAT_URIinstead of a hardcoded string for initiating a POST request is a good practice. It enhances maintainability and readability.
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
8 times, most recently
from
9748d71 to
b01ba1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 37
Actionable comments outside the diff hunks (37)
security/oc_pstat.c (3)
Line range hint
76-76: Replacememsetwithmemset_sto ensure buffer boundaries are respected, mitigating potential buffer overflow risks.- memset(g_pstat, 0, sizeof(g_pstat)); + memset_s(g_pstat, sizeof(g_pstat), 0, sizeof(g_pstat));
Line range hint
291-291: Replacememsetwithmemset_sin theoc_pstat_handle_statefunction to prevent potential buffer overflow issues.- memset(ps->rowneruuid.id, 0, sizeof(ps->rowneruuid.id)); + memset_s(ps->rowneruuid.id, sizeof(ps->rowneruuid.id), 0, sizeof(ps->rowneruuid.id));
Line range hint
494-494: Replacememsetwithmemset_sin theoc_sec_pstat_clearfunction to enhance security by respecting buffer boundaries.- memset(ps, 0, sizeof(*ps)); + memset_s(ps, sizeof(*ps), 0, sizeof(*ps));api/oc_core_res.c (3)
Line range hint
109-110: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(g_core_resources, 0, sizeof(g_core_resources)); - memset(g_oc_device_info, 0, sizeof(g_oc_device_info)); + memset_s(g_core_resources, sizeof(g_core_resources), 0, sizeof(g_core_resources)); + memset_s(g_oc_device_info, sizeof(g_oc_device_info), 0, sizeof(g_oc_device_info));
Line range hint
286-296: Replacememsetwithmemset_sin dynamic allocation scenarios to ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(device, 0, OC_NUM_CORE_LOGICAL_DEVICE_RESOURCES * sizeof(oc_resource_t)); - memset(&device_info[device_count], 0, sizeof(oc_device_info_t)); + memset_s(device, OC_NUM_CORE_LOGICAL_DEVICE_RESOURCES * sizeof(oc_resource_t), 0, OC_NUM_CORE_LOGICAL_DEVICE_RESOURCES * sizeof(oc_resource_t)); + memset_s(&device_info[device_count], sizeof(oc_device_info_t), 0, sizeof(oc_device_info_t));
Line range hint
431-431: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(&r->types, 0, sizeof(oc_string_array_t)); + memset_s(&r->types, sizeof(oc_string_array_t), 0, sizeof(oc_string_array_t));security/oc_doxm.c (2)
Line range hint
256-257: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(doxm->devowneruuid.id, 0, sizeof(doxm->devowneruuid.id)); - memset(doxm->rowneruuid.id, 0, sizeof(doxm->rowneruuid.id)); + memset_s(doxm->devowneruuid.id, sizeof(doxm->devowneruuid.id), 0, sizeof(doxm->devowneruuid.id)); + memset_s(doxm->rowneruuid.id, sizeof(doxm->rowneruuid.id), 0, sizeof(doxm->rowneruuid.id));
Line range hint
690-690: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(&decode, 0, sizeof(oc_doxm_decode_t)); + memset_s(&decode, sizeof(oc_doxm_decode_t), 0, sizeof(oc_doxm_decode_t));api/plgd/plgd_time.c (2)
Line range hint
518-518: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and enhance security.- memset(&pt, 0, sizeof(pt)); + memset_s(&pt, sizeof(pt), 0, sizeof(pt));
Line range hint
540-540: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and enhance security.- memset(&pt, 0, sizeof(pt)); + memset_s(&pt, sizeof(pt), 0, sizeof(pt));security/oc_acl.c (1)
Line range hint
582-582: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security against potential buffer overflows.- memset(&subject, 0, sizeof(oc_ace_subject_t)); + memset_s(&subject, sizeof(oc_ace_subject_t), 0, sizeof(oc_ace_subject_t));api/oc_collection.c (4)
Line range hint
818-818: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&response, 0, sizeof(oc_response_t)); + memset_s(&response, sizeof(oc_response_t), 0, sizeof(oc_response_t));This change addresses potential buffer overflow issues by ensuring that the size of the buffer is explicitly considered.
Line range hint
820-820: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&response_buffer, 0, sizeof(oc_response_buffer_t)); + memset_s(&response_buffer, sizeof(oc_response_buffer_t), 0, sizeof(oc_response_buffer_t));This change addresses potential buffer overflow issues by ensuring that the size of the buffer is explicitly considered.
Line range hint
822-822: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&rest_request, 0, sizeof(oc_request_t)); + memset_s(&rest_request, sizeof(oc_request_t), 0, sizeof(oc_request_t));This change addresses potential buffer overflow issues by ensuring that the size of the buffer is explicitly considered.
Line range hint
850-850: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&link_item, 0, sizeof(link_item)); + memset_s(&link_item, sizeof(link_item), 0, sizeof(link_item));This change addresses potential buffer overflow issues by ensuring that the size of the buffer is explicitly considered.
security/oc_cred.c (4)
Line range hint
271-271: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(&uuid, 0, sizeof(oc_uuid_t)); + memset_s(&uuid, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));
Line range hint
301-301: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(g_devices[device].rowneruuid.id, 0, OC_UUID_ID_SIZE); + memset_s(g_devices[device].rowneruuid.id, OC_UUID_ID_SIZE, 0, OC_UUID_ID_SIZE);
Line range hint
648-648: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(&public_key, 0, sizeof(oc_string_t)); + memset_s(&public_key, sizeof(oc_string_t), 0, sizeof(oc_string_t));
Line range hint
668-668: Replacememsetwithmemset_sto ensure buffer boundaries are respected, preventing potential buffer overflows.- memset(&subject, 0, sizeof(oc_uuid_t)); + memset_s(&subject, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));messaging/coap/observe.c (4)
Line range hint
807-807: Replacememsetwithmemset_s.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));The use of
memsetcan potentially lead to security vulnerabilities due to compiler optimizations that may remove these calls. Usingmemset_s, which is designed to be more secure against such optimizations, is recommended.
Line range hint
845-845: Replacememsetwithmemset_s.- memset(&request, 0, sizeof(request)); + memset_s(&request, sizeof(request), 0, sizeof(request));Similar to previous comments, replace
memsetwithmemset_sto enhance security by preventing potential buffer overflows and ensuring that the memory clearing is not optimized away by the compiler.
Line range hint
847-847: Replacememsetwithmemset_s.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));Continue replacing
memsetwithmemset_sfor the reasons mentioned earlier, enhancing the security of the code.
Line range hint
849-849: Replacememsetwithmemset_s.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));Ensure that buffer clearing operations are secure and not subject to compiler optimizations that could remove them.
api/oc_ri.c (7)
Line range hint
772-772: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(aux_arr, 0, sizeof(aux_arr)); + memset_s(aux_arr, sizeof(aux_arr), 0, sizeof(aux_arr));
Line range hint
830-830: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&handler, 0, sizeof(oc_request_handler_t)); + memset_s(&handler, sizeof(oc_request_handler_t), 0, sizeof(oc_request_handler_t));
Line range hint
1246-1246: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&result, 0, sizeof(result)); + memset_s(&result, sizeof(result), 0, sizeof(result));
Line range hint
1686-1686: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(preparsed_request_obj, 0, sizeof(oc_ri_preparsed_request_obj_t)); + memset_s(preparsed_request_obj, sizeof(oc_ri_preparsed_request_obj_t), 0, sizeof(oc_ri_preparsed_request_obj_t));
Line range hint
1741-1741: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
1745-1745: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_obj, 0, sizeof(response_obj)); + memset_s(&response_obj, sizeof(response_obj), 0, sizeof(response_obj));
Line range hint
1749-1749: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&request_obj, 0, sizeof(request_obj)); + memset_s(&request_obj, sizeof(request_obj), 0, sizeof(request_obj));security/oc_tls.c (5)
Line range hint
1325-1325: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&crt, 0, sizeof(mbedtls_x509_crt)); + memset_s(&crt, sizeof(mbedtls_x509_crt), 0, sizeof(mbedtls_x509_crt));
Line range hint
1837-1837: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&wildcard_sub, 0, sizeof(oc_uuid_t)); + memset_s(&wildcard_sub, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));
Line range hint
2018-2018: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&peer->timer, 0, sizeof(oc_tls_retry_timer_t)); + memset_s(&peer->timer, sizeof(oc_tls_retry_timer_t), 0, sizeof(oc_tls_retry_timer_t));
Line range hint
1437-1437: Replacestrncpywithstrcpy_sto ensure buffer boundaries are respected.- strncpy(credid_str, "any", sizeof(credid_str)); + strcpy_s(credid_str, sizeof(credid_str), "any");
Line range hint
1453-1453: Replacestrncpywithstrcpy_sto ensure buffer boundaries are respected.- strncpy(credid_str, "any", sizeof(credid_str)); + strcpy_s(credid_str, sizeof(credid_str), "any");security/oc_obt.c (2)
Line range hint
573-573: Replacememsetwithmemset_sto enhance security.- memset(&di, 0, sizeof(oc_uuid_t)); + memset_s(&di, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));Using
memset_sinstead ofmemsetcan help prevent potential buffer overflow issues and enhance the security of the code.
Line range hint
1425-1425: Usememset_sinstead ofmemsetfor better security practices.- memset(&subjectuuid, 0, sizeof(oc_uuid_t)); + memset_s(&subjectuuid, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));Replacing
memsetwithmemset_sprovides a safer alternative that considers buffer boundaries, reducing the risk of buffer overflows.
Danielius1922
force-pushed
the
adam/bugfix/sc-fixes-15
branch
from
b01ba1c to
7bd262b
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 5
Actionable comments outside the diff hunks (53)
api/cloud/oc_cloud_rd.c (1)
Line range hint
267-267: Avoid usingmemset()as it does not consider buffer boundaries and can lead to buffer overflows. Usememset_s()instead to enhance security.- memset(&partition, 0, sizeof(rd_links_partition_t)); + memset_s(&partition, sizeof(rd_links_partition_t), 0, sizeof(rd_links_partition_t));api/oc_etag.c (5)
Line range hint
248-248: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(&etag_map, 0, sizeof(etag_map)); + memset_s(&etag_map, sizeof(etag_map), 0, sizeof(etag_map));
Line range hint
524-526: Consider replacingmemsetwithmemset_sin multiple instances to ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(&request, 0, sizeof(request)); + memset_s(&request, sizeof(request), 0, sizeof(request)); - memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
571-571: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
589-589: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(&result, 0, sizeof(result)); + memset_s(&result, sizeof(result), 0, sizeof(result));
Line range hint
644-644: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));security/oc_pstat.c (3)
Line range hint
76-76: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(g_pstat, 0, sizeof(g_pstat)); + memset_s(g_pstat, sizeof(g_pstat), 0, sizeof(g_pstat));
Line range hint
291-291: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(ps->rowneruuid.id, 0, sizeof(ps->rowneruuid.id)); + memset_s(ps->rowneruuid.id, sizeof(ps->rowneruuid.id), 0, sizeof(ps->rowneruuid.id));
Line range hint
494-494: Consider replacingmemsetwithmemset_sto ensure buffer boundaries are respected and to prevent potential buffer overflows.- memset(ps, 0, sizeof(*ps)); + memset_s(ps, sizeof(*ps), 0, sizeof(*ps));api/oc_core_res.c (5)
Line range hint
109-109: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(g_core_resources, 0, sizeof(g_core_resources)); + memset_s(g_core_resources, sizeof(g_core_resources), 0, sizeof(g_core_resources));
Line range hint
110-110: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(g_oc_device_info, 0, sizeof(g_oc_device_info)); + memset_s(g_oc_device_info, sizeof(g_oc_device_info), 0, sizeof(g_oc_device_info));
Line range hint
286-287: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(device, 0, OC_NUM_CORE_LOGICAL_DEVICE_RESOURCES * sizeof(oc_resource_t)); + memset_s(device, OC_NUM_CORE_LOGICAL_DEVICE_RESOURCES * sizeof(oc_resource_t), 0, OC_NUM_CORE_LOGICAL_DEVICE_RESOURCES * sizeof(oc_resource_t));
Line range hint
296-296: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&device_info[device_count], 0, sizeof(oc_device_info_t)); + memset_s(&device_info[device_count], sizeof(oc_device_info_t), 0, sizeof(oc_device_info_t));
Line range hint
431-431: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&r->types, 0, sizeof(oc_string_array_t)); + memset_s(&r->types, sizeof(oc_string_array_t), 0, sizeof(oc_string_array_t));api/plgd/plgd_time.c (2)
Line range hint
518-518: Replacememsetwithmemset_sto ensure buffer boundaries are considered, enhancing security against potential buffer overflows.- memset(&pt, 0, sizeof(pt)); + memset_s(&pt, sizeof(pt), 0, sizeof(pt));
Line range hint
540-540: Replacememsetwithmemset_sto ensure buffer boundaries are considered, enhancing security against potential buffer overflows.- memset(&pt, 0, sizeof(pt)); + memset_s(&pt, sizeof(pt), 0, sizeof(pt));security/oc_acl.c (4)
Line range hint
582-582: Replacememset()withmemset_s()to ensure buffer boundaries are considered, preventing potential buffer overflows.- memset(&subject, 0, sizeof(oc_ace_subject_t)); + memset_s(&subject, sizeof(oc_ace_subject_t), 0, sizeof(oc_ace_subject_t));
Line range hint
600-600: Replacememset()withmemset_s()to ensure buffer boundaries are considered, preventing potential buffer overflows.- memset(&g_aclist[device].rowneruuid, 0, sizeof(oc_uuid_t)); + memset_s(&g_aclist[device].rowneruuid, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));
Line range hint
649-649: Replacememset()withmemset_s()to ensure buffer boundaries are considered, preventing potential buffer overflows.- memset(&subject, 0, sizeof(oc_ace_subject_t)); + memset_s(&subject, sizeof(oc_ace_subject_t), 0, sizeof(oc_ace_subject_t));
Line range hint
848-848: Replacememset()withmemset_s()to ensure buffer boundaries are considered, preventing potential buffer overflows.- memset(&_anon_clear, 0, sizeof(oc_ace_subject_t)); + memset_s(&_anon_clear, sizeof(oc_ace_subject_t), 0, sizeof(oc_ace_subject_t));api/oc_collection.c (4)
Line range hint
818-818: Replacememset()withmemset_s()to ensure buffer boundaries are respected and prevent potential buffer overflows.- memset(&response, 0, sizeof(oc_response_t)); + memset_s(&response, sizeof(oc_response_t), 0, sizeof(oc_response_t));
Line range hint
820-820: Replacememset()withmemset_s()to ensure buffer boundaries are respected and prevent potential buffer overflows.- memset(&response_buffer, 0, sizeof(oc_response_buffer_t)); + memset_s(&response_buffer, sizeof(oc_response_buffer_t), 0, sizeof(oc_response_buffer_t));
Line range hint
822-822: Replacememset()withmemset_s()to ensure buffer boundaries are respected and prevent potential buffer overflows.- memset(&rest_request, 0, sizeof(oc_request_t)); + memset_s(&rest_request, sizeof(oc_request_t), 0, sizeof(oc_request_t));
Line range hint
850-850: Replacememset()withmemset_s()to ensure buffer boundaries are respected and prevent potential buffer overflows.- memset(&link_item, 0, sizeof(link_item)); + memset_s(&link_item, sizeof(link_item), 0, sizeof(link_item));security/oc_cred.c (4)
Line range hint
271-271: Replacememsetwithmemset_sto enhance security by ensuring buffer boundaries are respected.- memset(&uuid, 0, sizeof(oc_uuid_t)); + memset_s(&uuid, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));
Line range hint
301-301: Replacememsetwithmemset_sto enhance security by ensuring buffer boundaries are respected.- memset(g_devices[device].rowneruuid.id, 0, OC_UUID_ID_SIZE); + memset_s(g_devices[device].rowneruuid.id, OC_UUID_ID_SIZE, 0, OC_UUID_ID_SIZE);
Line range hint
648-648: Replacememsetwithmemset_sto enhance security by ensuring buffer boundaries are respected.- memset(&public_key, 0, sizeof(oc_string_t)); + memset_s(&public_key, sizeof(oc_string_t), 0, sizeof(oc_string_t));
Line range hint
668-668: Replacememsetwithmemset_sto enhance security by ensuring buffer boundaries are respected.- memset(&subject, 0, sizeof(oc_uuid_t)); + memset_s(&subject, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));messaging/coap/observe.c (16)
Line range hint
807-807: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
845-845: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&request, 0, sizeof(request)); + memset_s(&request, sizeof(request), 0, sizeof(request));
Line range hint
847-847: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
849-849: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
923-923: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&request, 0, sizeof(request)); + memset_s(&request, sizeof(request), 0, sizeof(request));
Line range hint
925-925: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
927-927: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
998-998: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&request, 0, sizeof(request)); + memset_s(&request, sizeof(request), 0, sizeof(request));
Line range hint
1144-1144: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
1162-1162: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
1199-1199: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
1201-1201: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
1363-1363: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));
Line range hint
1397-1397: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
1548-1548: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response, 0, sizeof(response)); + memset_s(&response, sizeof(response), 0, sizeof(response));
Line range hint
1550-1550: Replacememsetwithmemset_sto ensure buffer boundaries are respected, enhancing security.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));api/oc_ri.c (7)
Line range hint
772-772: Replacememsetwithmemset_sfor safer memory initialization.- memset(aux_arr, 0, sizeof(aux_arr)); + memset_s(aux_arr, sizeof(aux_arr), 0, sizeof(aux_arr));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
Line range hint
830-830: Replacememsetwithmemset_sfor safer memory initialization.- memset(&handler, 0, sizeof(oc_request_handler_t)); + memset_s(&handler, sizeof(oc_request_handler_t), 0, sizeof(oc_request_handler_t));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
Line range hint
1246-1246: Replacememsetwithmemset_sfor safer memory initialization.- memset(&result, 0, sizeof(result)); + memset_s(&result, sizeof(result), 0, sizeof(result));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
Line range hint
1686-1686: Replacememsetwithmemset_sfor safer memory initialization.- memset(preparsed_request_obj, 0, sizeof(oc_ri_preparsed_request_obj_t)); + memset_s(preparsed_request_obj, sizeof(oc_ri_preparsed_request_obj_t), 0, sizeof(oc_ri_preparsed_request_obj_t));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
Line range hint
1741-1741: Replacememsetwithmemset_sfor safer memory initialization.- memset(&response_buffer, 0, sizeof(response_buffer)); + memset_s(&response_buffer, sizeof(response_buffer), 0, sizeof(response_buffer));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
Line range hint
1745-1745: Replacememsetwithmemset_sfor safer memory initialization.- memset(&response_obj, 0, sizeof(response_obj)); + memset_s(&response_obj, sizeof(response_obj), 0, sizeof(response_obj));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
Line range hint
1749-1749: Replacememsetwithmemset_sfor safer memory initialization.- memset(&request_obj, 0, sizeof(request_obj)); + memset_s(&request_obj, sizeof(request_obj), 0, sizeof(request_obj));This change ensures that the buffer boundaries are respected, preventing potential buffer overflow issues.
security/oc_obt.c (2)
Line range hint
573-573: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&di, 0, sizeof(oc_uuid_t)); + memset_s(&di, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));
Line range hint
1425-1425: Replacememsetwithmemset_sto ensure buffer boundaries are respected.- memset(&subjectuuid, 0, sizeof(oc_uuid_t)); + memset_s(&subjectuuid, sizeof(oc_uuid_t), 0, sizeof(oc_uuid_t));
| @@ -106,7 +101,7 @@ get_new_aceid(size_t device) | |||
| return aceid; | |||
| } | |||
|
|
|||
| static oc_ace_res_t * | |||
| oc_ace_res_t * | |||
There was a problem hiding this comment.
Given the complexity of access control logic and the critical role it plays in security, it's essential to have comprehensive unit tests covering various scenarios, including different types of subjects (UUID, role, connection type), permissions, and resource specifications (specific resources, wildcards). This will help ensure that the access control logic behaves as expected across a wide range of cases.
Would you like assistance in creating a detailed test plan or implementing unit tests for these changes?
|
Summary by CodeRabbit
"security/oc_acl_util_internal.h".