Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

πŸ›‘οΈ Halt Operation can lead to DOS #624

Closed
ccamel opened this issue May 15, 2024 · 1 comment Β· Fixed by #710
Closed

πŸ›‘οΈ Halt Operation can lead to DOS #624

ccamel opened this issue May 15, 2024 · 1 comment Β· Fixed by #710
Assignees
Labels
security audit Categorizes an issue or PR as relevant to Security Audit

Comments

@ccamel
Copy link
Member

ccamel commented May 15, 2024

Note

Severity: Info
target: v7.1.0 - Commit: 3c854270b006db30aa3894da2cdba10cc31b8c5f
Ref: OKP4 Blockchain Audit Report v1.0 - 02-05-2024 - BlockApex

Description

Okp4 supports rich set of prolog predicates. These predicates allows to define complex business logic and agreements conditions which can be queried and evaluated on-chain. Okp4 provides options to whitelist and blacklist the set of prolog predicates. One such predicates which caught our specific attention during our audit was halt/1. As per the swi-prolog documentation.

Halt: Terminate Prolog execution with default exit code using halt/1. The default exit code is normally 0, but can be 1 if one of the Prolog flags on_error or on_warning is set to status and there have been errors or warnings.

If the Blockchain is bootstrapped using the default configuration and 'halt' is specifically not blacklisted opens the room for potential usage of the predicate which would result is the termination of the underlaying node.

Recommandation

If the halt/1 is executed it would result in the termination of the blockchain process.

Although Okp4 explicitly mentions the potential impact of the halt/1 butwe find it necessary to mention that since okp4 recommend it to blacklist this predicate, we propose that this predicate should be removed from the code because the potential impact of someone not blacklisting is very critical as it would result in the terminating of the Node.

@ccamel ccamel added the security audit Categorizes an issue or PR as relevant to Security Audit label May 15, 2024
@github-project-automation github-project-automation bot moved this to πŸ“‹ Backlog in πŸ’» Development May 15, 2024
@github-project-automation github-project-automation bot moved this to πŸ“‹ Backlog in πŸ’» Development May 15, 2024
@ccamel ccamel moved this from πŸ“‹ Backlog to πŸ“† To do in πŸ’» Development May 15, 2024
@ccamel
Copy link
Member Author

ccamel commented Jun 16, 2024

I agree with the recommandation to exclude potentially dangerous built-in predicates provided by ichiban/prolog (like halt/1) from the registry of available predicates. Although the whitelist/blacklist exclusion mechanism offers some control, it is not absolute and can be overridden through governance. Furthermore, given the potential impact of these predicates, there is no justification for their existence in the protocol.

I'm therefore in favour of the following strategic approach: only declare native predicates from the logic module in the registry. This allows:

  • Restrict predicate origin: Limit the source of predicates to our logic module exclusively.
  • Exercise selective control: Maintain precise control over the predicates registered, ensuring that dangerous ones (like halt/1) are excluded.
  • Enhance documentation: Publish comprehensive documentation for all registered predicates using the existing documentation generation mechanism.

The predicates from ichiban/prolog that we want to integrate as they are into the registry must therefore be re-exported in the logic module and properly documented. This is already our practice, as demonstrated with the consult/1 predicate:

// x/logic/predicate/file.go
// ... proper documentation ...
func Consult(vm *engine.VM, file engine.Term, cont engine.Cont, env *engine.Env) *engine.Promise {
    return engine.Consult(vm, file, cont, env)
}

@ccamel ccamel self-assigned this Jun 18, 2024
@ccamel ccamel moved this from πŸ“† To do to πŸ— In progress in πŸ’» Development Jun 18, 2024
@github-project-automation github-project-automation bot moved this from πŸ“‹ Backlog to βœ… Done in πŸ’» Development Jul 26, 2024
@github-project-automation github-project-automation bot moved this from πŸ— In progress to βœ… Done in πŸ’» Development Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security audit Categorizes an issue or PR as relevant to Security Audit
Projects
Status: βœ… Done
Development

Successfully merging a pull request may close this issue.

1 participant