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

Consideration for Memcache Injection Vulnerabilities #1791

Closed
ImanSharaf opened this issue Nov 21, 2023 · 16 comments · Fixed by #1856
Closed

Consideration for Memcache Injection Vulnerabilities #1791

ImanSharaf opened this issue Nov 21, 2023 · 16 comments · Fixed by #1856
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@ImanSharaf
Copy link
Collaborator

Memcache, a widely-used distributed memory caching system, can be vulnerable to injection attacks. These vulnerabilities arise when untrusted data is concatenated into a Memcache command without proper handling or sanitization. The impact of such vulnerabilities can range from data leakage to unauthorized access or modification of cached data. Reference:

$key = $_POST['key'];
$value = $_POST['value'];

// The attacker can send a key that includes a command or query
// For example: "key; delete users; key"

$memcache = new Memcache();
$memcache->connect('localhost', 11211);
$memcache->set($key, $value);

// The malicious command or query will be stored in the cache and may be executed later

Proposed requirement:

  • Ensure that data sent to Memcache servers is properly validated and sanitized to prevent Memcache Injection attacks.
@elarlang
Copy link
Collaborator

elarlang commented Nov 22, 2023

How the proposal is different from current 8.1.1?

# Description L1 L2 L3 CWE
8.1.1 [MODIFIED, MERGED FROM 8.1.2] Verify that the application prevents sensitive data from being cached in server components such as load balancers and application caches or ensures that the data is securely purged after use. 524

edit: but if I watch your code example, then there the problem is not caching as such, but not validated/sanitized value for key. This part is covered in general Input validation and Sanitization requirements.

In general I smell the usual "testing checklist" vs "list of requirements for reporting" mindset difference here.

@elarlang elarlang added the 2) Awaiting response Awaiting a response from the original poster label Nov 22, 2023
@ImanSharaf
Copy link
Collaborator Author

These two are completely different!!! Memcache Injection represents a specific attack vector that arises from the unique way Memcache interprets concatenated commands. This risk is distinct from merely caching sensitive data.

@elarlang
Copy link
Collaborator

These two are completely different!!!

Well, there is a bit difference in your proposal as well:

Ensure that data sent to Memcache servers is properly validated and sanitized to prevent Memcache Injection attacks.

May comment was for this proposal - this proposed text is quite similar to 8.1.1 requirement - the end goal is the same.

Now when I watched your code example, it was a bit different problem there than proposed requirement.

@ImanSharaf
Copy link
Collaborator Author

ImanSharaf commented Nov 22, 2023

I would like to highlight some key distinctions that make the inclusion of specific guidelines for Memcache Injection distinct from what is currently covered in 8.1.1.

Firstly, requirement 8.1.1 primarily addresses the prevention of sensitive data being cached in server components and ensuring secure purging of data post-use. While important, this requirement doesn't explicitly cover the validation and sanitization of data being used in cache operations, especially in the context of Memcache.

The core of Memcache Injection attacks lies in the manipulation of cache keys or values, which is not directly addressed in 8.1.1. The proposed requirement emphasizes the need for specific handling of data sent to Memcache servers, focusing on validating and sanitizing the data used in Memcache commands to prevent attackers from executing malicious operations.

I encourage you to review the nature of Memcache Injection attacks.

@elarlang
Copy link
Collaborator

elarlang commented Nov 22, 2023

I was going to write the answer, but... everything I started to write is already written. Sometimes I wonder do people read before they post comments... :)

For (re)starting point:

edit: but if I watch your code example, then there the problem is not caching as such, but not validated/sanitized value for key. This part is covered in general Input validation and Sanitization requirements.

In general I smell the usual "testing checklist" vs "list of requirements for reporting" mindset difference here.

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet and removed 2) Awaiting response Awaiting a response from the original poster labels Nov 22, 2023
@Sjord
Copy link
Contributor

Sjord commented Nov 23, 2023

I think 5.3.4 is more applicable to memcache injection:

5.3.4 Verify that data selection or database queries (e.g. SQL, HQL, ORM, NoSQL) use parameterized queries, ORMs, entity frameworks, or are otherwise protected from database injection attacks.

There are many requirements already to prevent against specific types of attacks:

  • 5.2.3, SMTP or IMAP injection
  • 5.2.5, template injection
  • 5.3.4, 5.3.5, SQL injection
  • 5.3.6, JSON injection
  • 5.3.7, LDAP injection
  • 5.3.8, 12.3.5, command injection
  • 5.3.10, XPath injection or XML injection
  • 7.3.1, log injection

You could say, "memcache injection is missing from this list!", and you would be right. But I think it is pointless to try to exhaustively list all systems which would be vulnerable to injection. I would rather see more general requirements that work against all forms of injection.

$memcache->set($key, $value);

This shouldn't be vulnerable to injection. The set function itself should do the escaping, not the caller. I think it does and this blog post is talking nonsense. Memcache injection is possible, but not with this code example.

The blog post also suggests using prepared statements, but memcache doesn't have those. It also seems to confuse memcache and memcached.

@ImanSharaf
Copy link
Collaborator Author

@Sjord I understand your point about the potential confusion between 'memcache' and 'memcached.' To clarify:

Memcached is the specific open-source distributed memory caching system. It's the actual software that runs as a daemon and provides the caching functionality.

Memcache, on the other hand, is often used more generally to refer to the concept or methodology of caching objects in memory to speed up applications. Additionally, in some programming languages, like PHP, there are libraries named 'Memcache' used to interface with the Memcached service.

Regarding with the set function, this method is not typically designed to perform escaping or sanitization of input data. Its focus is on caching efficiency, not security.

Let's go for a real world example for Memcache Injection, to simplify this, a user's routing information is stored like this:

user_email = "user@example.com"
cache[user_email] = "server1"

An attacker finds a way to inject a newline character and additional command. Now, the cache has an entry that not only stores the attacker's route, but also executes the malicious command which changes the route for the user to an attacker-controlled server.

I'm curious to understand the reasoning behind the viewpoint that including Memcache Injection in the ASVS would be pointless. Could you elaborate on how we distinguish between vulnerabilities like LDAP or IMAP Injection, which are deemed valid for inclusion, and Memcache Injection, which is considered not necessary to include?

@elarlang elarlang added the V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements label Nov 27, 2023
@tghosth tghosth added the _5.0 - prep This needs to be addressed to prepare 5.0 label Jan 24, 2024
@tghosth
Copy link
Collaborator

tghosth commented Jan 24, 2024

If we separately include things like LDAP injection and IMAP injection, I agree that we can't really ignore memcache injection. I do however think it is highly likely that at some point we will need to compress these requirements into less requirements.

In the meantime, is there any sanitization required other than preventing newlines @ImanSharaf?

@Sjord
Copy link
Contributor

Sjord commented Jan 24, 2024

I think the best solution for memcache and other forms of injections is to use a library with an API that automatically encodes/escapes/validates. The application does not have to take care of injection anymore, since this is abstracted away by the library. PHP's memcache APIs already do this, so

$memcache->set($key, $value);

is not vulnerable. The problem occurs if the application talks to memcache directly and creates its own commands by concatenating strings.

I think "use a high-level API instead of talking to the other system directly" can be a nice requirement, solving many security problems.

@elarlang
Copy link
Collaborator

I think "use a high-level API instead of talking to the other system directly" can be a nice requirement, solving many security problems.

From the requirement point of view, it applies to the last bit which communicates with an external component (in this case Memcache).

@tghosth
Copy link
Collaborator

tghosth commented Jan 24, 2024

Thoughts on:

Verify that the application uses a high-level API which performs input sanitization instead of communicating directly with a memcache instance to prevent injection attacks. Alternatively, ensure that content is carefully sanitized before being sent to memcache.

@elarlang
Copy link
Collaborator

I don't like the direction, I think we should concentrate only on preventing security holes from happening not giving architecture (and not directly security-related) advice.

@elarlang elarlang added the next meeting Filter for leaders label Jan 24, 2024
@tghosth
Copy link
Collaborator

tghosth commented Jan 25, 2024

Having discussed with @elarlang, I think we don't need to be too prescriptive...

Verify that the application sanitizes content before it is being sent to memcache to prevent injection attacks.

Any final thoughts?

@tghosth tghosth removed the next meeting Filter for leaders label Jan 25, 2024
@ImanSharaf
Copy link
Collaborator Author

Having discussed with @elarlang, I think we don't need to be too prescriptive...

Verify that the application sanitizes content before it is being sent to memcache to prevent injection attacks.

Any final thoughts?

It works for me.

@elarlang elarlang added the 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR label Jan 30, 2024
@elarlang
Copy link
Collaborator

elarlang commented Feb 3, 2024

The situation at the moment:

  • requirement: Verify that the application sanitizes content before it is being sent to memcache to prevent injection attacks.
  • category/section: "V5.2 Sanitization and Sandboxing"
  • level: it depends, how we going to describe levels, at the moment I would say 2
  • cwe: there is no precise match, canditates are listed in the "data neutralization issues" list, but I can not see anything to match. Descriptions and titles are quite specific for other problems. So, at the moment I would leave it empty.

@tghosth
Copy link
Collaborator

tghosth commented Feb 5, 2024

Opened #1856 to add this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants