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

Bugs: Some potential NULL dereference bugs #1915

Open
icy17 opened this issue Oct 12, 2023 · 7 comments · May be fixed by #1921
Open

Bugs: Some potential NULL dereference bugs #1915

icy17 opened this issue Oct 12, 2023 · 7 comments · May be fixed by #1921

Comments

@icy17
Copy link
Contributor

icy17 commented Oct 12, 2023

Description

There are some potential null dereference bugs.
In src/Mayaqua/Network.c: 5807 and 5671, calling SSL_set_ex_data without checking the parameter 1 might cause a null-dereference.

In src/Mayaqua/Encrypt.c: 778, calling BN_bn2bin without checking the parameter 2 might cause a null-dereference.

Expected behavior:
It's better to check the pointer before calling these APIs

@chipitsine
Copy link
Member

indeed SSL_new may return NULL.

how did you find it ? mind submit a PR ?

@icy17
Copy link
Contributor Author

icy17 commented Oct 12, 2023

I'm detecting API misuse by static analysis. I'll submit a PR later.

@chipitsine
Copy link
Member

interesting. how do you detect api misuse ?

@icy17
Copy link
Contributor Author

icy17 commented Oct 13, 2023

For example, for SSL_set_ex_data:

  1. Generate security rules of an API: After thoroughly reviewing the API documentation and examining some code, I found that the parameter 1 of SSL_set_ex_data must not be set to NULL, as doing so may lead to a system crash.
  2. Write Detection code based on the security rules: I wrote detection code based on this principle. The primary logic of the detection code is checking whether the API is checked (like if (ssl != NULL)) before it's called.
  3. Detection: I use CodeQL to run this detection code on some applications.

@icy17 icy17 linked a pull request Oct 24, 2023 that will close this issue
@icy17
Copy link
Contributor Author

icy17 commented Oct 24, 2023

hi! I'm not sure if there should be a null check before SSL_get_ex_data. So I did not fix it in this PR.
image

@chipitsine
Copy link
Member

do you mind sharing CodeQL queries ? (or is it a "know how" ?)

@icy17
Copy link
Contributor Author

icy17 commented Nov 8, 2023

/**
 * @name parameterCheck
 * @description description
 * @kind problem
 * @problem.severity error
 * @precision high
 * @id cpp/paracheck
 * @tags security
 */

 import cpp
 import semmle.code.cpp.dataflow.TaintTracking
 import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.security.Security
 import semmle.code.cpp.controlflow.Guards
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
     
 Expr getSinkExpr(FunctionCall fc)
 {
     //Change
 result = fc.getArgument(0) 
 }
 
 predicate isSinkFC(FunctionCall fc)
 {
     // Change
 fc.getTarget().hasName("SSL_CTX_set_options")
 }
 GuardCondition getGuard(FunctionCall fc) {
    isSinkFC(fc)
    and
     exists(Expr e, Variable a| e = getSinkExpr(fc)
     and a.getAnAccess() = e
     and exists(GuardCondition g, Expr ge| 
         a.getAnAccess() = ge
         and g.getASuccessor*() = fc
         and g.getAChild*() = ge
         and result = g
         )
     )
 }
 
 
 from FunctionCall target
 where
 // get target functioncall
 isSinkFC(target)
// filter fc (no guardCondition check like if(para != NULL) before calling the fc)
 and not exists(GuardCondition g| 
     g = getGuard(target)
     )
// only check local var
and exists(Expr e, LocalVariable a| e = getSinkExpr(target)
 and a.getAnAccess() = e.getAChild*()
)
// make sure the target parameter is not like: &var
and not exists(AddressOfExpr ae | 
    ae = getSinkExpr(target))
 select target, target.getLocation().toString()

It's a example CodeQL detection code to check if there is a check of parameter 1 before calling SSL_CTX_set_options. This code is very basic, and I haven't implemented a highly precise detection code to avoid potential performance overhead. If you want to detect other bugs related to other functionCall and other parameter, you just need to change the name SSL_CTX_set_options and 0 in this line result = fc.getArgument(0)
I think understanding this code may require some knowledge of CodeQL's ql query syntax statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants