Add security and confidentiality analysis - #2883
Conversation
lwrage
left a comment
There was a problem hiding this comment.
I found four correctness issues that should be addressed before merging: nonterminating traversal of feedback paths, missed hidden flows through nested components, security checks skipped when caveats are absent, and binding-label state retained across analyses.
These findings are based on static inspection of the PR and surrounding implementation, checked against the cited security-analysis report where applicable. I did not run builds or tests. Regression coverage should exercise these cases through valid AADL models.
| FeatureInstance destination = fsi.getDestination(); | ||
| if (destination != null) { | ||
| // Follow the subcomponents flow destination | ||
| unWalkedFeatures.add(destination); |
There was a problem hiding this comment.
[P1] Track visited features so feedback paths terminate
getDestinations() repeatedly queues downstream features without recording which ones have already been visited; getSources() has the same problem. A reachable cycle through subcomponent flow paths continually repopulates the queue, so the analysis never completes. For example, an input can reach two subcomponents whose output-to-input connections form a feedback loop, even when another branch reaches the enclosing component's output. Add cycle detection to both traversals and cover a reachable feedback loop with a regression.
| */ | ||
| List<FeatureInstance> getDestinations(ComponentInstance ci, FeatureInstance inPort) { | ||
| LinkedList<FeatureInstance> destinations = new LinkedList<>(); | ||
| EList<ConnectionInstance> connections = ci.getConnectionInstances(); |
There was a problem hiding this comment.
[P1] Preserve intermediate boundary features when discovering hidden flows
This traversal uses complete connection instances and later compares their endpoints directly with the component's boundary features. For a wrapper forwarding its input and output to an internal thread, the complete connections can belong to the enclosing system and terminate at the thread's ports. Starting at the wrapper's input then finds no matching connection, so a missing flow declaration on the wrapper escapes detection. The reverse traversal has the same limitation. Traverse connection references in the component's context so the intermediate boundary features remain visible.
| } | ||
| return Optional.of(new SecurityLabel(level, tags)); | ||
| } catch (Exception e) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
[P1] Do not silently discard an explicit security level when caveats are absent
The contributed Security_Level_Caveats property has no default. When it is absent, this catch returns an empty label even if Security_Level was explicitly assigned. An untrusted flow with TopSecret input and Confidential output, but no caveats assignments, therefore receives no flow-security error: checkValidFlow() sees both labels as absent and reports neither the downgrade nor missing labels. Provide the intended defaults or diagnose incomplete labels explicitly instead of silently skipping the checks. The cited report specifies lowest-level and empty-category defaults in section 3.1.1: https://www.sei.cmu.edu/documents/1290/2021_005_001_652223.pdf
|
|
||
| componentLabel = LabelUtil.getLabel(src) != null ? LabelUtil.getLabel(src) : Optional.empty(); | ||
| } | ||
| highestBindingLabel = LabelUtil.join(highestBindingLabel, componentLabel); |
There was a problem hiding this comment.
[P2] Reset binding aggregates before each propagation
This joins current labels with highestBindingLabel already stored on the model's EMF adapters, but that field is never reset between modes or analysis runs. The UI analyzes successive system operation modes using the same model. If a processor hosts TopSecret software in one mode and only Confidential software in another, the second analysis retains the earlier requirement and can suppress its least-privilege warning. Reset the binding aggregates before propagation, and verify that diagnostics for a mode do not depend on which mode was analyzed first.
This pull request adds the security and confidentiality analysis as presented in 'Modeling and Validating Security and
Confidentiality in System Architectures' DOI: 10.1184/R1/13659911.
All rules and additional checks mentioned in the report are implemented to the best of my knowledge.
As stated in the report section 4.2.1 (subprogram calls):
Therefore Rule 4, is not included in this implementation.
I've included some examples for getting started/show-casing and testing the functionality. A proper unit test has not been written.
Additionally, I've looked at how other analyses are added to the project and used those as a guideline for adding this analysis.
I managed to successfully run
build.localon a Windows machine resulting in an executable containing the newly added features.