Ruby: Add rb/tainted-format-string query

[hmac]

This shares as much as possible with the JS version.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output or throw an exception.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output or throw an exception.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output or throw an exception.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf throw an exception that there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, a method such as Kernel.puts should be used, which does not apply string formatting to its arguments.

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output or throw an exception.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf will throw an exception as there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, string interpolation should be used exclusively:

class UsersController < ActionController::Base
  def index
    puts "Unauthorised access attempt by #{params[:user]}: #{request.ip}"
  end
end

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.
• Common Weakness Enumeration: CWE-134.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output or throw an exception.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf will throw an exception as there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, string interpolation should be used exclusively:

class UsersController < ActionController::Base
  def index
    puts "Unauthorised access attempt by #{params[:user]}: #{request.ip}"
  end
end

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.

[erik-krogh]

JS 👍

[alexrford]

Looks good, only minor comments.

[alexrford]

We should probably either change this to be 0-indexed rather than 1-indexed, or note in the QLDoc that it's 1-indexed.

[alexrford]

Suggested change

	IOPrintfCall() { this = API::getTopLevelMember("IO").getInstance().getAMethodCall("printf") }
	IOPrintfCall() { this.getReceiver() instanceof IOInstance and this.getMethodName() = "printf" }

with an import of codeql.ruby.frameworks.Files::IO.

This will catch cases like:

    file = File.open "foo.txt", "a"
    file.printf(params[:format], arg) # BAD

where the IO instance is created without new - this case is already caught by the UnknownMethodCall catch-all as it stands, but that might not be the case if/when UnknownMethodCall is removed.

[alexrford]

As it stands we can end up with FPs of the form:

    printf("hello %s %s", params[:format], "arg2")

where params[:format] is currently interpreted as a format string due to appearing as arg1, but is only a format argument.

I think we could avoid this by checking the type of arg0, e.g. as a rough heuristic:

Suggested change

	override DataFlow::Node getFormatString() { result = this.getArgument([0, 1]) }
	override DataFlow::Node getFormatString() {
	if this.getArgument(0).asExpr() instanceof StringLiteralCfgNode
	then result = this.getArgument(0)
	else result = this.getArgument([0, 1])
	}

In this way, we avoid FPs where arg0 is trivially a format string, but we err on the side of over-reporting when we're unsure of the type of arg0.

[hmac]

Good point! I've made this change, though instead of instanceof StringLiteralCfgNode I've used .getConstantValue().isString(_), to take advantage of local flow, e.g. in

fmt = "hello %s %s"
printf(fmt, params[:format], "arg2")

This relies on IO objects not having a string constant value, but I think that's a safe bet.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Use of externally-controlled format string

Methods like Kernel.printf accept a format string that is used to format the remaining arguments by providing inline format specifiers. If the format string contains unsanitized input from an untrusted source, then that string may contain unexpected format specifiers that cause garbled output or throw an exception.

Recommendation

Either sanitize the input before including it in the format string, or use a %s specifier in the format string, and pass the untrusted data as corresponding argument.

Example

The following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to Kernel.printf to be appended to the message:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by #{params[:user]}: %s", request.ip)
  end
end

However, if a malicious user provides a format specified such as %s as their user name, Kernel.printf will throw an exception as there are too few arguments to satisfy the format. This can result in denial of service or leaking of internal information to the attacker via a stack trace.

Instead, the user name should be included using the %s specifier:

class UsersController < ActionController::Base
  def index
    printf("Unauthorised access attempt by %s: %s", params[:user], request.ip)
  end
end

Alternatively, string interpolation should be used exclusively:

class UsersController < ActionController::Base
  def index
    puts "Unauthorised access attempt by #{params[:user]}: #{request.ip}"
  end
end

References

• Ruby documentation for format strings.
• Common Weakness Enumeration: CWE-134.

[alexrford]

👍

[erik-krogh]

I think this import semmle.javascript.security.dataflow.DOM is redundant.
But that's fine.
(ql/redundant-import anyone?)

[hmac]

A query for redundant imports would be great! Even better would be a compiler warning delivered via LSP.

[erik-krogh]

I don't think it belongs as a compiler warning.
In the above case both of the imports eventually import each other, so deleting any one of them will solve it.

And do you want a compiler warning on both for that? I think that feels wrong, and so does the arbitrary choice of warning on the second.
And sometimes (redundant) imports and their order can be important (I'm not sure why, but I think the CPP team can tell stories).

In QL-for-QL: arbitary choices and lingering warnings are fine, so I think it should be a query.
(But I want to figure out how we'll get support for parameterized modules before I start more QL-for-QL query writing).