Subject: [TLS] The MCSV hack is a compliment to SSL/TLS designers

From: James Manger <[email-masked]>
Date: Thu, 10 Dec 2009 23:59:46 +1100

The Magic Cipher Spec Value (MCSV) is a hack.
An empty RI extension is also a hack.
The only non-hack solution is a new version number, but that is impractical.

The designers of SSL/TLS should take the fact that a hack is needed to fix this bug as a compliment. It indicates that SSL/TLS has been so successful, is used by so many application protocols, on so many devices, in so many situations that minimising disruption to this unknowable collection of systems trumps all other criteria for choosing a fix (other than actually fixing the bug, of course). It trumps architectural purity; it trumps saving a few bytes; it trumps a few weeks heads start in developing code.

I recommend the approach in draft-mrex-tls-secure-renegotiation-03 (MCSV in every ClientHello).

This approach simply takes out of the equation all the uncertainties about the prevalence, behaviour, and importance or otherwise of old versions or limited implementations.

-- 
James Manger

---

From: Yoav Nir <[email-masked]>
Date: Thu, 10 Dec 2009 15:38:41 +0200

+!

---

From: Marsh Ray <[email-masked]>
Date: Thu, 10 Dec 2009 10:54:26 -0600

> The Magic Cipher Spec Value (MCSV) is a hack.

Yes.

> An empty RI extension is also a hack.

No, it's a well-defined and appropriate way of communicating an
important bit of information over the protocol, namely: "I support the
renegotiation fix and my belief is that we have had no previous
connection state on this connection".

> The only non-hack solution is a new version number, but that is

Which is why most well-designed nontrivial protocols have an extension
mechanism.

> The designers of SSL/TLS should take the fact that a hack is needed
> to fix this bug as a compliment.

I wouldn't.

> It indicates that SSL/TLS has been so successful, is used by so many
> application protocols, on so many devices, in so many situations that
> minimising disruption to this unknowable collection of systems trumps
> all other criteria for choosing a fix (other than actually fixing the
> bug, of course).

It has been very successful yes, and that is at least in part because
the designers did many things right.

> It trumps architectural purity; it trumps saving a few bytes; it
> trumps a few weeks heads start in developing code.

That's not a good thing.

> (MCSV in every ClientHello).

It also requires the use of an extension.

> This approach simply takes out of the equation all the uncertainties
> about the prevalence, behaviour, and importance or otherwise of old
> versions or limited implementations.

No it doesn't.

- Marsh

---

From: Martin Rex <[email-masked]>
Date: Thu, 10 Dec 2009 18:22:09 +0100 (MET)

> It also requires the use of an extension.

No, it does NOT.

draft-mrex-tls-secure-renegotiation-03 uses additional data following
the compression_method in ServerHello for Server->Client signaling.
See Section 4.4 last two paragraphs of that draft.

That extra data is designed to be compatible with TLS extensions,
but it can be processed like a static 6-octet string by extensions-free
implementations.

Clients that _send_ TLS extensions in ClientHello will have to search
the S->C signal among the TLS extensions in ServerHello -- but that
is an obvious side-effect of asking for TLS extension in ClientHello.

-Martin

---

From: Marsh Ray <[email-masked]>
Date: Thu, 10 Dec 2009 12:46:33 -0600

> but it can be processed like a static 6-octet string by extensions-free

Hmmm, an additional six bytes after a hello:

0x00  0x04  0xFF  0x01  0x00  0x00

I'd call that an extension.

The only difference is that in draft-ietf-tls-renegotiation it contains
useful payload data on renegotiation handshakes.

> is an obvious side-effect of asking for TLS extension in ClientHello.

Right.

And in draft-ietf-tls-renegotiation you have to be able to iterate
extensions on any hello message. Not a big deal. In any case, you've had
six and a half years to prepare yourself for it.

Hmm, let's see how long it takes me to write this in C++.

Starting typing code at 11:41 AM
Finished primary functiat at 11:59.

So the basic "wade through the list of extensions" function came out to
51 LoC (including comments and gratuitous whitespace) and 18 minutes.

Wonder if it compiles?

Fixed typo assert(extId); should be assert(extType);

Fixed type error
	*rv.assign(p, p + statedExtDataLen);
should be
	rv->assign(p, p + statedExtDataLen);

(I updated the version pasted above)

Compiles cleanly at 12:04, time includes typing into this email.

Quick and dirty test program with actual data from Wireshark. Stepping
through in debugger.

12:15 PM Found bug. Missing loop increment, silly mistake.

12:28 PM Passes four reasonably-useful test cases.

So it took 47 minutes including writing a custom unit test. I would
probably stare at it a while longer and find things I'd want to change.
I would definitely look at it again the next day. QA will want to work
on product tests of course.

There is a non-zero probability that it still contains bugs, but I think
it's a reasonable first cut.

Anyone is welcome to use it if it helps.

- Marsh
//	This example code which is quite possibly defective is placed
//	in the public domain by the author Marsh Ray without making any
//	representation that it is suitable for any purpose.

#include <cassert>
#include <exception>
#include <vector>
#include <memory> // auto_ptr
using namespace std;

typedef std::exception MyRuntimeExceptionType; // define according to your needs

//	Obtains the payload of the specified extension.
//	Returns a null auto_ptr if the extension was not present.
auto_ptr<vector<unsigned char> > getExtensionData(
   unsigned char const * const pBeginExtData,  // in, first byte of extension data
   unsigned char const * const pEndExtData,  // in, first position _after_ extension data
   unsigned const extType ) // in, IANA-assigned type id of extension of interest
{
	assert(pBeginExtData && pBeginExtData < pEndExtData);
	assert(extType);

    auto_ptr<vector<unsigned char> > rv; // returned value

    for (unsigned char const * p = pBeginExtData; true; )
	{
		size_t const cntBytesAvail = pEndExtData - p;

		if (cntBytesAvail == 0)
			break; // Cleanly reached end of extension data.

		else if (cntBytesAvail < 4) // A minimal extension is 4 bytes.
			throw MyRuntimeExceptionType(); // Bad extension data.

		else
		{
			// We have a plausible amount of extension data.

			unsigned const statedExtType = (p[0] << 8) | p[1];
			p += 2;

			unsigned const statedExtDataLen = (p[0] << 8) | p[1];
			p += 2;

			if (p + statedExtDataLen > pEndExtData)
				throw MyRuntimeExceptionType(); // Bad extension data stated length.

			if (statedExtType == extType)
			{
				// Hooray, we found it!
				rv.reset(new vector<unsigned char>);

				if (statedExtDataLen > 0)
					rv->assign(p, p + statedExtDataLen);

				break;
			}

			// Advance to next extension.
			p += statedExtDataLen;
		}
	}

    return rv;
}

int main(int argc, char *argv[])
{
	int rv = -1;

	try
	{
		//	From actual packets
		static unsigned char extBlock[] = {
			0x00, 0x0b, 0x00, 0x04, 0x03, 0x00, 0x01, 0x02,

			0x00, 0x0a, 0x00, 0x34, 0x00, 0x32, 0x00, 0x01, 0x00, 0x02,
			0x00, 0x03, 0x00, 0x04, 0x00, 0x05, 0x00, 0x06,
			0x00, 0x07, 0x00, 0x08, 0x00, 0x09, 0x00, 0x0a,
			0x00, 0x0b, 0x00, 0x0c, 0x00, 0x0d, 0x00, 0x0e,
			0x00, 0x0f, 0x00, 0x10, 0x00, 0x11, 0x00, 0x12,
			0x00, 0x13, 0x00, 0x14, 0x00, 0x15, 0x00, 0x16,
			0x00, 0x17, 0x00, 0x18, 0x00, 0x19,
			
			0x00, 0x23, 0x00, 0x00 };

		//---- Test case: Nonempty extension, first in list
		{
			auto_ptr<vector<unsigned char> > extData = getExtensionData(
			   extBlock, // unsigned char const * const pBeginExtData,
			   extBlock + sizeof(extBlock), // unsigned char const * const pEndExtData,
			   0x000b ); // unsigned const extType

			if (!extData.get()) throw std::runtime_error("test fail");
			if (extData->size() != 4) throw std::runtime_error("test fail");
			if (extData->at(0) != 0x03) throw std::runtime_error("test fail");
			if (extData->at(3) != 0x02) throw std::runtime_error("test fail");
		}

		//---- Test case: Nonempty extension not first in list
		{
			auto_ptr<vector<unsigned char> > extData = getExtensionData(
			   extBlock, // unsigned char const * const pBeginExtData,
			   extBlock + sizeof(extBlock), // unsigned char const * const pEndExtData,
			   0x000a ); // unsigned const extType

			if (!extData.get()) throw std::runtime_error("test fail");
			if (extData->size() != 52) throw std::runtime_error("test fail");
			if (extData->at(1) != 0x32) throw std::runtime_error("test fail");

		}

		//---- Test case: Empty extension, last in sequence.
		{
			auto_ptr<vector<unsigned char> > extData = getExtensionData(
			   extBlock, // unsigned char const * const pBeginExtData,
			   extBlock + sizeof(extBlock), // unsigned char const * const pEndExtData,
			   0x0023 ); // unsigned const extType

			if (!extData.get()) throw std::runtime_error("test fail");
			if (extData->size() != 0) throw std::runtime_error("test fail");
		}

		//---- Test case: Extension not found.
		{

			auto_ptr<vector<unsigned char> > extData = getExtensionData(
			   extBlock, // unsigned char const * const pBeginExtData,
			   extBlock + sizeof(extBlock), // unsigned char const * const pEndExtData,
			   0x9999 ); // unsigned const extType

			if (extData.get()) throw std::runtime_error("test fail");
		}

		rv = 0;
	}
	catch (std::exception & e) { cout << "Exception: " << e.what() << "\n"; }
	catch (...) { cout << "Exception.\n"; }

	cout << "Done.\n";

	return rv;
}

---

From: Martin Rex <[email-masked]>
Date: Thu, 10 Dec 2009 21:10:10 +0100 (MET)

> I'd call that an extension.

To an extension-less TLS client and TLS server this is a fixed, 6-octet
piece of binary data.

To extensions-aware TLS clients and TLS servers this is a single
TLS extension.

To avoid confusion with TLS extension RI, my openssl patch uses 0xff 0x02
(the I-D itself only contains TBD).

> useful payload data on renegotiation handshakes.

TLS extension RI uses an _much_ more complex sematic, which has been
causing the obvious confusion among implementers what to send on the
renegotiation ClientHello in a backwards interop scenario, and some
are actually fighting for the wrong answer.

> Hmm, let's see how long it takes me to write this in C++.

In good mission-critical software, the error handling is 70-80% of
the code.  Your code avoids this issue for the most part.

Your approach is doing what I call module testing.
There should be more test vectors, in particular defective
test vectors, plus checking that the error handlings works
correctly (completely missing in your example).

If your error handling is using C++ exceptions, that this part
can _NOT_ be tested by module testing in this fashion (that would
only work embedded in the actual TLS implementation that does
the exceptinon processing.

What you didn't count so far is all the work that is necessary to
automate this testing, in particular if you want to do module
testing on the code when it is part of the TLS implementation,
which might include test scripts, Makefiles, new test targets, ...

Btw. unconditional assert() is something that you _DEFINITELY_ do not
want in mission critical software.  A crash/core-dump is not
permitted behaviour in productive environments.  Instead, if
you detect a situation that prevents you from continuing,
the code should try to fail sensibly and gracefully, which means
returning an error and trying to write a log/trace entry reporting
the situation.

If you have a server running with 200 concurrent user sessions,
and there is one situation where one of your libraries gets
called with a NULL object handle, you do _NOT_ want to crash
the entire server, causing all 200 users to loose all their
current state (including unsaved data), but instead, you
report an error to the caller and write a log/trace file entry.

When stuff like this happens at a customer site, it might be an
indicator that the callers above your code might not have sufficient
test coverage.

If you need to be able to run a debugger on the customers machine
in order to analyze an error a complex piece of application software,
then there are some environments where you will have a hard time
selling (or create a huge amount of maintenance costs).

Personally, I like the "writing code" part also  _much_ more
than the writing TESTs part (in particular because the latter
is several times more work).  When you are forced to write
your own tests (or take _full_ responsibility when your code
breaks at the customers site), then you may start writing code
in a fashion that it is easily testable, robust and failSAFE.

-Martin

Off-topic: using C++ for something that is a toolkit for consumption
by others is a decision you will regret badly after some years on
almost every Unix platform. (hints: shared libraries, dynamic runtime
loading, global symbol space -- there are tons of platfrom-specific
suprises waiting to be discovered).

---

From: Marsh Ray <[email-masked]>
Date: Thu, 10 Dec 2009 16:57:31 -0600

> To an extension-less TLS client and TLS server this is a fixed, 6-octet

Until someone else wants that space for their own "fixed piece of binary
data".

> the code.  Your code avoids this issue for the most part.

My code throws an exception when it determines the input is invalid or
memory allocation fails. It's standard C++ stuff and works pretty well
in my experience.

A full C++ TLS implementation would probably have its own type for
reporting protocol errors. It would probably already have error handling
code with an exception-based interface.

> the exceptinon processing.

Sure, but it should fit easily with the established model.

> want in mission critical software.

That's why production releases are compiled with #define NDEBUG.

> A crash/core-dump is not

No it doesn't "mean" that.

Exceptions are superior to returning errors, in part because they are
fail-safe.

In any case this stuff is regularly discussed to death on comp.lang.c++
and other lists.

> report an error to the caller and write a log/trace file entry.

What does this have to do with anything?

Did you find a crash bug in my code?

> suprises waiting to be discovered).

LOL.  Dude, you don't have to use my C++ if you don't want it.

I've certainly had some regrets on the *nix side over the years but C++
has never been one of them.

- Marsh