-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code review general refactoring #96
Conversation
…nected to avoid confusion.
…terparts for uniformity.
- clarify and uniformise info texts - homogenise attack step patterns - homogenise attack step names - replace British spelling with its American counterparts - remove unhelpful info strings - flag attack steps as entry points See #91 for more details
…he ConnectionRule logic and be clearer Changed also the ClientAccess association to NetworkClientAccess to highlight the network aspect of the client
…ormity and clarity
… deny for uniformity.
…es modify which is the preferred way.
…vent all of the relevant attack steps.
src/main/mal/DataResources.mal
Outdated
@@ -88,96 +45,116 @@ category DataResources { | |||
|
|||
| extract | |||
user info: "The attacker is able to extract the information. This means that they have a path available through which they can transfer the information back to a system they control." | |||
|
|||
// The following 10 attack steps are used to implement data replication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there 10 attack steps after this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were before I removed 5 of them in a later commit. Funny catch. :)
developer info: "Reverse reach is used to determine whether or not the attacker can be reached by the user." | ||
-> clientApplications().attemptReverseReach, | ||
developer info: "Reverse reach is used to determine whether or not the attacker can be reached by the user. Reverse reach propagates via outgoing or bidirectional communications." | ||
-> senderApplications().attemptReverseReach, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking on the old code, we had clientApplications().attemptReverseReach
here and we now have senderApplications().attemptReverseReach
. But isn't receiverApplications
the equivalent of clientApplications
? If that is the case then I messed up and renamed them the wrong way around :O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am ultra confused right now. Previously we had let clientApplications = (applications \/ outApplications)
so that means that the Applications
that are having an outbound ConnectionRule
were the clients?
This became now: let senderApplications = (applications \/ outApplications)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think your changes are correct, reverse reach propagates towards sender applications, it goes the opposite direction as regular network connectivity.
… language, and actually use the inboundAllowedConnections variable.
… used for data replication.
This is the massive code review pull request. See #91 for a list of issues meant to be solved by the pulls request.