Meta protocol flush_all method is vulnerable to code injection (Lack of input type check)

[xhzeem]

Hi there,
I'm a security researcher currently doing research on Memcached wrappers vulnerabilities. I was doing some source code reviewing on your wrapper and noticed that the flush_all method on the meta protocol takes a delay value and passes it to the server without any checks, which can be used to smuggle commands to the Memcached server if an attacker has control over the value passed to the flush_all method.

dalli/lib/dalli/protocol/meta.rb

Lines 137 to 140 in 5588d98

	def flush(delay = 0)
	write(RequestFormatter.flush(delay: delay))
	response_processor.flush unless quiet?
	end

dalli/lib/dalli/protocol/meta/request_formatter.rb

Lines 76 to 81 in 5588d98

	def self.flush(delay: nil, quiet: false)
	cmd = +'flush_all'
	cmd << " #{delay}" if delay
	cmd << ' noreply' if quiet
	cmd + TERMINATOR
	end

Proof of Concept

require 'dalli'

# Proof of Concept by @xhzeem
$mcmeta = Dalli::Client.new('localhost:11211', protocol: :meta)
$mcmeta.set('xhzeem','meta')
$mcmeta.get("xhzeem") # b64_if_reg_match(\s)
puts $mcmeta.flush_all("\nset xhzeem 1 1000 8\ninjected") # :D

Suggested Fix:

You should just add a simple check for the delay type and confirm it's a number or keep the 0 value.

[petergoldstein]

Thanks @xhzeem - very helpful. While I'm not sure that the threat surface here is that large (if you have control over the flush_all argument you probably have control over the Dalli client), but I agree that this should be sanitized. I'll get a fix in shortly.

[xhzeem]

Thanks for your fast response...
Indeed the impact isn't very high for this case as the flush_all method will not be controlled by the user in most cases, in a case where a user sets a timer to restore or similar.

Anyway, the point here is not about being able to write ruby code, but instead passing unsafe user-controlled data to that method which can be used in some cases by malicious actors to inject more commands into the Memcached server.

[xhzeem]

I see the fix is sufficient.
Nice job

[xhzeem]

I’ll do some final checks today and confirm I cannot find any other similar issues

[xhzeem]

I'm unable to get some tests done right so if you can help to confirm

dalli/lib/dalli/protocol/meta.rb

Line 111 in 5588d98

def delete(key, cas)

How should I call this method and pass the cas?

dalli/lib/dalli/protocol/meta.rb

Line 46 in 5588d98

def touch(key, ttl)

The touch function is lacking the ttl = TtlSanitizer.sanitize(ttl) if ttl part, but as far as my tests it is still only accepting numbers in the TTL, I cannot get the flow exactly.

If you can confirm those two methods are safe I think everything else is good so far as of my checks.
delete => cas variable
touch => ttl variable

[petergoldstein]

@xhzeem I'll take a look.

I don't think Dalli does any material checking on the format of the CAS, but memcached does require the CAS to be a 64-bit value. So that's an easy sanitize. I'll add that and a corresponding test.

Regarding touch, the ttl is sanitized at the Dalli::Client level I believe.

[xhzeem]

Good job I've checked the lastest version and to me, everything looks great.