Signing undefined values: signature_mismatch

[Igor Gariev]

Greetings,
currently (revision 160) server and consumer interpret missing
arguments differently, that leads to signature_mismatch error.

To calculate/check signature, a token string (list of "key:value"
pairs joined by new line) is constructed, where keys are listed in
'openid.signed' param.

Server skips missing values completely (Net/OpenID/Server.pm, line
294):
foreach my $f (@sign) {
next unless defined $arg{$f};
$token_contents .= "$f:$arg{$f}\n";
...

Consumer uses empty strings instead of missed values, and adds strings
like "key:" to token (Net/OpenID/Consumer.pm, line 789):
foreach my $param (split(/,/, $signed)) {
my $val = $self->args("openid.$param");
$token .= "$param:$val\n";

Signatures of (thus) different tokens certainly don't match. I had the
problem with 'op_endpoint' parameter (Net/OpenID/Server.pm, line 269)

Regretfully, OpenID specification says nothing about how to sign
missing values.

Possible solutions:

1. Don't put keys of undefied/empty values into 'openid.signed' list
at all. Pro: this will solve all problems of portability between
different libraries/languages. Con: it's not possible to sign empty
values (to sign that some values is missing), so intruder may put any
value instead of missing one.

2. Both client and server must process all keys in 'openid.signed'
list and treat missing values as empty strings. Pro: this is secure
solution, that seems to work with previous version of at least Perl
library. Con: since specification says nothing, it's not guaranteed
that all implementation will follow this practice.

Diff for #2 is below:
--- Net-OpenID-Server/lib/Net/OpenID/Server.pm (revision 160)
+++ Net-OpenID-Server/lib/Net/OpenID/Server.pm (working copy)
@@ -291,7 +291,6 @@
my @arg; # arguments we'll append to the URL
my $token_contents = "";
foreach my $f (@sign) {
- next unless defined $arg{$f};
$token_contents .= "$f:$arg{$f}\n";
push @arg, "openid.$f" => $arg{$f};
delete $arg{$f};

[Martin Atkins]

Igor Gariev wrote:
>
> 1. Don't put keys of undefied/empty values into 'openid.signed' list
> at all. Pro: this will solve all problems of portability between
> different libraries/languages. Con: it's not possible to sign empty
> values (to sign that some values is missing), so intruder may put any
> value instead of missing one.
>

This is the solution I'd favor. Note that the specification requires
certain fields to be signed, so a message that includes, for example, a
return_to argument but does not include return_to in the signature
should be treated by Consumer as an error. If that is not the case, then
we should fix that in Consumer.

> Regretfully, OpenID specification says nothing about how to sign
> missing values.
>

I guess the specification assumes that missing values will not be
included in openid.signed at all, but it would of course be better if it
gave some specific guidance here. This would be worth posting to the
OpenID Specs list so that it can be included in the next version of the
specification.

[Igor Gariev]

Ok, here is the patch for solution #1:

--- perl/Net-OpenID-Server/lib/Net/OpenID/Server.pm (revision 160)
+++ perl/Net-OpenID-Server/lib/Net/OpenID/Server.pm (working copy)
@@ -285,13 +285,14 @@
push @sign, $k;
}

- # include the list of all fields we'll be signing
+ # since signing of empty fields is not well defined,
+ # remove such fields from the list of fields to be signed
+ @sign = grep { defined $arg{$_} && $arg{$_} ne '' } @sign;
$arg{signed} = join(",", @sign);

my @arg; # arguments we'll append to the URL
my $token_contents = "";
foreach my $f (@sign) {
- next unless defined $arg{$f};
$token_contents .= "$f:$arg{$f}\n";
push @arg, "openid.$f" => $arg{$f};
delete $arg{$f};

And here is the patch for Consumer.
Specification of OpenID 1.1 doesn't list fields that must be signed,
so the check is for version 2.0 only.

--- perl/Net-OpenID-Consumer/lib/Net/OpenID/Consumer.pm (revision 160)
+++ perl/Net-OpenID-Consumer/lib/Net/OpenID/Consumer.pm (working copy)
@@ -777,7 +777,28 @@
my $assoc = Net::OpenID::Association::handle_assoc($self,
$server, $assoc_handle);

my %signed_fields; # key (without openid.) -> value
-
+
+ # Specificaiton v. 2.0 has strict requirements on which keys must
be signed
+ if ($self->_message_version>=2) {
+ my %signed_fields = map {$_ => 1} split /,/, $signed;
+ my %unsigned_fields;
+ # these fields must be signed unconditionally
+ foreach my $f (qw/op_endpoint return_to response_nonce
assoc_handle/) {
+ $unsigned_fields{$f}++ if !$signed_fields{$f};
+ }
+ # these fields must be signed if present
+ foreach my $f (qw/claimed_id identity/) {
+ next unless $self->args("openid.$f");
+ $unsigned_fields{$f}++ if !$signed_fields{$f};
+ }
+ if (%unsigned_fields) {
+ return $self->_fail(
+ "unsigned_field",
+ "Field(s) must be signed: " . join(", ", keys
%unsigned_fields)
+ );
+ }
+ }
+
if ($assoc) {
$self->_debug("verified_identity: verifying with found
association");

I've tested the new patches in the following use-cases:
patched Consumer accepts the Google OpenID and ID from Net::OpenID
Perl server
patched Server works with Net::OpenID consumer and with blogger.com
(doesn't work without the patch!)

Martin, we need the changes (at least server part) for LiveJournal, it
would be nice if you could include them into trunk soon.
I can send the patch as an e-mail attachment, or whatever you'll find
appropriate.