Improve validation of keyring DID and Name (fixes #131)

Correct the maximum DID length defined in "serval_types.h" from 32 to 31. Add a definition of the maximum identity Name length and use it instead of the bare constant 64, eg, in the MDP_DNALOOKUP request handling code. Introduce a dataformats.h function for validating an identity name, and use it to validate the 'name' parameter in the CLI 'keyring set' command. Add 'did' and 'name' parameter validation to the GET /restful/keyring/add and GET /restful/keyring/SID/set requests (#131). Rename keyring_set_did() to keyring_set_did_name() and assert that DID and Name lengths have been validated before storing in the keyring. Update the Keyring REST API tech document.
2024-12-18 12:56:29 +00:00 · 2017-12-19 12:39:29 +10:30 · 2017-12-19 12:39:29 +10:30 · c3cf86161f
commit c3cf86161f
parent afa33484ba
13 changed files with 166 additions and 96 deletions
--- a/cli.c
+++ b/cli.c
@ -490,3 +490,8 @@ int cli_optional_did(const char *text)
 {
  return text[0] == '\0' || str_is_did(text);
 }
+
+int cli_optional_identity_name(const char *text)
+{
+  return text[0] == '\0' || str_is_identity_name(text);
+}
--- a/cli.h
+++ b/cli.h
@ -93,6 +93,7 @@ int cli_optional_bundle_crypt_key(const char *arg);
 int cli_interval_ms(const char *arg);
 int cli_uint(const char *arg);
 int cli_optional_did(const char *text);
+int cli_optional_identity_name(const char *text);

 /* Output functions.  Every command that is invoked via the CLI must use
 * exclusively the following primitives to send its response.
--- a/dataformats.c
+++ b/dataformats.c
@ -273,7 +273,7 @@ int is_didchar(char c)

 int strn_is_did(const char *did, size_t *lenp)
 {
-  int i;
+  size_t i;
  for (i = 0; i < DID_MAXSIZE && is_didchar(did[i]); ++i)
    ;
  if (i < DID_MINSIZE)
@ -283,6 +283,24 @@ int strn_is_did(const char *did, size_t *lenp)
  return 1;
 }

+int str_is_identity_name(const char *name)
+{
+  size_t len = 0;
+  return strn_is_identity_name(name, &len) && name[len] == '\0';
+}
+
+int strn_is_identity_name(const char *name, size_t *lenp)
+{
+  size_t i;
+  for (i = 0; i < ID_NAME_MAXSIZE && name[i]; ++i)
+    ;
+  if (i < ID_NAME_MINSIZE)
+    return 0;
+  if (lenp)
+    *lenp = i;
+  return 1;
+}
+
 void write_uint64(unsigned char *o,uint64_t v)
 {
  int i;
--- a/dataformats.h
+++ b/dataformats.h
@ -25,6 +25,8 @@ int str_is_identity(const char *sid);
 int strn_is_identity(const char *sid, size_t *lenp);
 int str_is_did(const char *did);
 int strn_is_did(const char *did, size_t *lenp);
+int str_is_identity_name(const char *name);
+int strn_is_identity_name(const char *name, size_t *lenp);

 int rhizome_strn_is_manifest_id(const char *text);
 int rhizome_str_is_manifest_id(const char *text);
--- a/dna_helper.c
+++ b/dna_helper.c
@ -105,7 +105,7 @@ parseDnaReply(const char *buf, size_t len, char *token, char *did, char *name, c
  *p = '\0';
  if (b == e || *b++ != '|')
    return 0;
-  for (p = name, q = name + 63; b != e && *b != '|' && p != q; ++p, ++b)
+  for (p = name, q = name + ID_NAME_MAXSIZE; b != e && *b != '|' && p != q; ++p, ++b)
    *p = *b;
  *p = '\0';
  if (b == e || *b++ != '|')
@ -428,7 +428,7 @@ void handle_reply_line(const char *bufp, size_t len)
    } else {
      char sidhex[SID_STRLEN + 1];
      char did[DID_MAXSIZE + 1];
-      char name[64];
+      char name[ID_NAME_MAXSIZE + 1];
      char uri[512];
      const char *replyend = NULL;
      if (!parseDnaReply(bufp, len, sidhex, did, name, uri, &replyend))
--- a/doc/REST-API-Keyring.md
+++ b/doc/REST-API-Keyring.md
@ -1,6 +1,6 @@
 Keyring REST API
 ================
-[Serval Project][], November 2017
+[Serval Project][], December 2017

 Introduction
 ------------
@ -71,21 +71,25 @@ will only be one kind of Serval ID, which will be a great simplification.
 ### DID

 The **DID** ([Dialled Identity][]) is a telephone number, represented as a
-string of five or more digits from the set `123456789#0*`.  It is used by the
-[DNA][] protocol to allow [Serval mesh network][] users to discover each other
-by telephone number; the first step in establishing a mesh voice call.
+string of between 5 and 31 ASCII characters from the set `123456789#0*`.  It is
+used by the [DNA][] protocol to allow [Serval mesh network][] users to discover
+each other by telephone number; the first step in establishing a mesh voice
+call.

 ### Name

-The **Name** is a short, non-blank, non-empty, unstructured string assigned by
-a human user to an identity.  It is used to represent the identity to human
-users, as it is more recognisable than a hexadecimal [SID](#serval-id) or a
-[DID](#did) (telephone number).
+The **Name** is a short, unstructured string between 1 and 63 bytes in length,
+assigned by a human user to an identity.  It is used to represent the identity
+to human users, as it is more recognisable than a hexadecimal [SID](#serval-id)
+or a [DID](#did) (telephone number).

-The name is encoded using [UTF-8][].  Since it is intended for human
-consumption, it may be constrained to contain only printable characters and no
-carriage-motion characters (eg, TAB U+0009 or LF U+0010), and to not start or
-end with white space.
+Serval DNA does not interpret the name, merely stores it, so the name may use
+any encoding on which all clients agree, such as ASCII or [UTF-8][].  Since it
+is intended for human consumption, it is recommended that it contain only
+printable characters, that it contain no carriage-motion characters (eg, TAB
+U+0009 or LF U+0010), and that it not start or end with white space, but Serval
+DNA does not enforce any such rules.  The only restriction enforced by Serval
+DNA is that it contain no zero bytes.

 ### Rhizome Secret

@ -113,7 +117,7 @@ remain locked and unusable forever.  There is no “master PIN” or back-door.
 ### Identity unlocking

 All Keyring requests can supply a passphrase using the optional **pin**
-parameter, which unlocks all keyring identities protected by that password,
+parameter, which unlocks all keyring identities protected by that passphrase,
 prior to performing the request.  Serval DNA caches every PIN it receives until
 the PIN is revoked using the [lock request](#get-restful-keyring-lock), so once
 an identity is unlocked, it remains visible until explicitly locked.
@ -151,31 +155,44 @@ Keyring REST API operations

 ### GET /restful/keyring/identities.json

-Returns a list of all currently unlocked identities, in [JSON table][] format.
+Returns a list of all currently unlocked identities, one identity per row in
+[JSON table][] format.  The following parameters are recognised:
+
+*   **pin**: see [identity unlocking](#identity-unlocking)
+
 The table columns are:

-*   **sid**: the [SID](#serval-id) of the identity, a string of 64 uppercase
-    hex digits
-*   **identity**: the [Signing ID](#serval-signing-id) of the identity, a
-    string of 64 uppercase hex digits
-*   **did**: the optional [DID](#did) (telephone number) of the identity;
-    `null` if none is assigned
-*   **name**: the optional string [Name](#name) of the identity; `null` if none
-    is assigned
+| heading    | content                                                                   |
+|:---------- |:------------------------------------------------------------------------- |
+| `sid`      | the [SID](#serval-id), a string of 64 uppercase hex digits                |
+| `identity` | the [Signing ID](#serval-signing-id), a string of 64 uppercase hex digits |
+| `did`      | the optional [DID](#did) (telephone number); `null` if none is assigned   |
+| `name`     | the optional string [Name](#name); `null` if none is assigned             |

 ### GET /restful/keyring/add

-Creates a new identity with a random [SID](#serval-id).  If the **pin**
-parameter is supplied, then the new identity will be protected by that
-password, and the password will be cached by Serval DNA so that the new
-identity is unlocked.
+Creates a new identity with a random [SID](#serval-id).  The following
+parameters are recognised:

-Returns [201 Created][201] if an identity is created; the [JSON
+*   **pin**: if present, then the new identity is protected by the given
+    passphrase; see [identity unlocking](#identity-unlocking) -- note that the
+    newly created identity is already unlocked when this request returns,
+    because the passphrase has been added to the PIN cache
+*   **did**: the DID (phone number); empty or absent to indicate no DID,
+    otherwise must conform to the rules for [DID](#did)
+*   **name**: the name; empty or absent to specify no name, otherwise must
+    conform to the rules for [Name](#name)
+
+If any parameter contains an invalid value then the request returns [400 Bad
+Request][400].  Returns [201 Created][201] if an identity is created; the [JSON
 result](#keyring-json-result) describes the identity that was created.

 ### GET /restful/keyring/SID/remove

-Removes an existing identity with a given [SID](#serval-id).
+Removes an existing identity with a given [SID](#serval-id).  The following
+parameters are recognised:
+
+*   **pin**: see [identity unlocking](#identity-unlocking)

 If there is no unlocked identity with the given SID, this request returns [404
 Not Found][404].  Otherwise it returns [200 OK][200] and the [JSON
@ -183,15 +200,19 @@ result](#keyring-json-result) describes the identity that was removed.

 ### GET /restful/keyring/SID/set

-Sets the [DID](#did) and/or name of the unlocked identity that has the given
-[SID](#serval-id).  The following parameters are recognised:
+Sets and/or clears the [DID](#did) and/or [Name](#name) of the unlocked
+identity that has the given [SID](#serval-id).  The following parameters are
+recognised:

-*   **did**: sets the DID (phone number); must be a string of five or more
-    digits from the set `123456789#0*`
-*   **name**: sets the name; must be non-empty
+*   **pin**: see [identity unlocking](#identity-unlocking)
+*   **did**: the DID (phone number); empty to clear the DID, otherwise must
+    conform to the rules for [DID](#did)
+*   **name**: the name; empty to clear the name, otherwise must conform to the
+    rules for [Name](#name)

-If there is no unlocked identity with the given SID, this request returns [404
-Not Found][404].
+If any parameter contains an invalid value then the request returns [400 Bad
+Request][400].  If there is no unlocked identity with the given SID, this
+request returns [404 Not Found][404].

 ### GET /restful/keyring/SID/lock

--- a/keyring.c
+++ b/keyring.c
@ -1,5 +1,5 @@
 /*
-Copyright (C) 2016 Flinders University
+Copyright (C) 2016-2017 Flinders University
 Copyright (C) 2013-2015 Serval Project Inc.
 Copyright (C) 2010-2012 Paul Gardner-Stephen
 
@ -170,8 +170,8 @@ static int keyring_load(keyring_file *k, const char *pin)

 void keyring_iterator_start(keyring_file *k, keyring_iterator *it)
 {
-  bzero(it, sizeof(keyring_iterator));
  assert(k);
+  bzero(it, sizeof(keyring_iterator));
  it->file = k;
 }

@ -706,9 +706,12 @@ static int unpack_cryptobox(keypair *kp, struct rotbuf *rb, size_t key_length)

 static int pack_did_name(const keypair *kp, struct rotbuf *rb)
 {
+  // Ensure DID is nul terminated.
+  if (strnchr((const char *)kp->private_key, kp->private_key_len, '\0') == NULL)
+    return WHY("DID missing nul terminator");
  // Ensure name is nul terminated.
  if (strnchr((const char *)kp->public_key, kp->public_key_len, '\0') == NULL)
-    return WHY("missing nul terminator");
+    return WHY("Name missing nul terminator");
  return pack_private_public(kp, rb);
 }

@ -716,8 +719,10 @@ static int unpack_did_name(keypair *kp, struct rotbuf *rb, size_t key_length)
 {
  if (unpack_private_public(kp, rb, key_length) == -1)
    return -1;
-  // Fail if name is not nul terminated.
-  return strnchr((const char *)kp->public_key, kp->public_key_len, '\0') == NULL ? -1 : 0;
+  // Fail if DID and Name are not nul terminated.
+  return    strnchr((const char *)kp->private_key, kp->private_key_len, '\0') != NULL
+	 && strnchr((const char *)kp->public_key, kp->public_key_len, '\0') != NULL
+	 ? 0 : -1;
 }

 static void dump_did_name(const keypair *kp, XPRINTF xpf, int UNUSED(include_secret))
@ -816,11 +821,15 @@ const struct keytype keytypes[] = {
      .loader = load_private_only
    },
  [KEYTYPE_DID] = {
-      /* The DID is stored in unpacked form in the private key field, and the name in nul-terminated
-       * ASCII form in the public key field.
+      /* The DID is stored in nul-terminated unpacked form in the private key field, and the name in
+       * nul-terminated ASCII form in the public key field.
       */
-      .private_key_size = 32,
-      .public_key_size = 64,
+      // DO NOT define the following size fields directly using DID_MAXSIZE or ID_NAME_MAXSIZE,
+      // which define the Serval DNA API, but not the keyring file format.  This is to avoid the
+      // risk that changing the API might (unintentionally) alter the keyring format, leading to
+      // non-back-compatible breakage.
+      .private_key_size = 32, // should be >= DID_MAXSIZE + 1
+      .public_key_size = 64, // should be >= ID_NAME_MAXSIZE + 1
      .packed_size = 32 + 64,
      .creator = NULL, // not included in a newly created identity
      .packer = pack_did_name,
@ -1603,41 +1612,45 @@ int keyring_commit(keyring_file *k)
  return errorCount ? WHYF("%u errors commiting keyring to disk", errorCount) : 0;
 }

-int keyring_set_did(keyring_identity *id, const char *did, const char *name)
+int keyring_set_did_name(keyring_identity *id, const char *did, const char *name)
 {
  /* Find where to put it */
  keypair *kp = id->keypairs;
  while(kp){
    if (kp->type==KEYTYPE_DID){
-      DEBUG(keyring, "Identity already contains DID");
+      DEBUG(keyring, "Identity already contains DID/Name");
      break;
    }
    kp=kp->next;
  }
  
-  /* allocate if needed */
+  /* Allocate if not found */
  if (!kp){
    if ((kp = keyring_alloc_keypair(KEYTYPE_DID, 0)) == NULL)
      return -1;
    keyring_identity_add_keypair(id, kp);
-    DEBUG(keyring, "Created DID record for identity");
+    DEBUG(keyring, "Created DID/Name record for identity");
  }

-  /* Store DID unpacked for ease of searching */
-  size_t len=strlen(did);
-  if (len>31)
-    len=31;
-  bcopy(did,&kp->private_key[0],len);
-  bzero(&kp->private_key[len],32-len);
-  len=strlen(name);
-  if (len>63)
-    len=63;
-  bcopy(name,&kp->public_key[0],len);
-  bzero(&kp->public_key[len],64-len);
+  /* Store DID as nul-terminated string. */
+  {
+    size_t len = strlen(did);
+    assert(len < kp->private_key_len);
+    bcopy(did, &kp->private_key[0], len);
+    bzero(&kp->private_key[len], kp->private_key_len - len);
+  }
+
+  /* Store Name as nul-terminated string. */
+  {
+    size_t len = strlen(name);
+    assert(len < kp->public_key_len);
+    bcopy(name, &kp->public_key[0], len);
+    bzero(&kp->public_key[len], kp->public_key_len - len);
+  }

  if (IF_DEBUG(keyring)) {
-    dump("{keyring} storing did",&kp->private_key[0],32);
-    dump("{keyring} storing name",&kp->public_key[0],64);
+    dump("{keyring} storing DID",&kp->private_key[0],32);
+    dump("{keyring} storing Name",&kp->public_key[0],64);
  }
  return 0;
 }
--- a/keyring.h
+++ b/keyring.h
@ -123,7 +123,7 @@ keyring_file *keyring_create_instance();
 keyring_file *keyring_open_instance(const char *pin);
 keyring_file *keyring_open_instance_cli(const struct cli_parsed *parsed);
 int keyring_enter_pin(keyring_file *k, const char *pin);
-int keyring_set_did(keyring_identity *id, const char *did, const char *name);
+int keyring_set_did_name(keyring_identity *id, const char *did, const char *name);
 int keyring_set_pin(keyring_identity *id, const char *pin);
 int keyring_sign_message(struct keyring_identity *identity, unsigned char *content, size_t buffer_len, size_t *content_len);
 int keyring_send_identity_request(struct subscriber *subscriber);
--- a/keyring_cli.c
+++ b/keyring_cli.c
@ -268,11 +268,9 @@ static int app_keyring_set_did(const struct cli_parsed *parsed, struct cli_conte
  
  if (cli_arg(parsed, "sid", &sidhex, str_is_subscriber_id, "") == -1 ||
      cli_arg(parsed, "did", &did, cli_optional_did, "") == -1 ||
-      cli_arg(parsed, "name", &name, NULL, "") == -1)
+      cli_arg(parsed, "name", &name, cli_optional_identity_name, "") == -1)
    return -1;
  int set_pin = cli_arg(parsed, "new_pin", &new_pin, NULL, "") == 0;
-  if (strlen(name)>63)
-    return WHY("Name too long (31 char max)");

  sid_t sid;
  if (str_to_sid_t(&sid, sidhex) == -1){
@ -286,8 +284,8 @@ static int app_keyring_set_did(const struct cli_parsed *parsed, struct cli_conte
  keyring_identity *id = keyring_find_identity_sid(keyring, &sid);
  if (!id)
    return WHY("No matching SID");
-  if (keyring_set_did(id, did, name))
-    return WHY("Could not set DID");
+  if (keyring_set_did_name(id, did, name) == -1)
+    return WHY("Could not set DID/Name");
  if (set_pin && keyring_set_pin(id, new_pin))
    return WHY("Could not set new pin");
  if (keyring_commit(keyring))
--- a/keyring_restful.c
+++ b/keyring_restful.c
@ -24,6 +24,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 #include "server.h"
 #include "keyring.h"
 #include "strbuf_helpers.h"
+#include "dataformats.h"

 DEFINE_FEATURE(http_rest_keyring);

@ -219,15 +220,21 @@ static int restful_keyring_add(httpd_request *r, const char *remainder)
  const char *pin = http_request_get_query_param(&r->http, "pin");
  const char *did = http_request_get_query_param(&r->http, "did");
  const char *name = http_request_get_query_param(&r->http, "name");
+  if (did && did[0] && !str_is_did(did))
+    return http_request_keyring_response(r, 400, "Invalid DID");
+  if (name && name[0] && !str_is_identity_name(name))
+    return http_request_keyring_response(r, 400, "Invalid Name");
  keyring_identity *id = keyring_create_identity(keyring, pin ? pin : "");
  if (id == NULL)
    return http_request_keyring_response(r, 500, "Could not create identity");
-  if (did || name){
-    if (keyring_set_did(id, did ? did : "", name ? name : "") == -1)
-      return http_request_keyring_response(r, 500, "Could not set identity DID/Name");
+  if ((did || name) && keyring_set_did_name(id, did ? did : "", name ? name : "") == -1) {
+    keyring_free_identity(id);
+    return http_request_keyring_response(r, 500, "Could not set identity DID/Name");
  }
-  if (keyring_commit(keyring) == -1)
+  if (keyring_commit(keyring) == -1) {
+    keyring_free_identity(id);
    return http_request_keyring_response(r, 500, "Could not store new identity");
+  }
  return http_request_keyring_response_identity(r, 201, id);
 }

@ -256,12 +263,16 @@ static int restful_keyring_set(httpd_request *r, const char *remainder)
  const char *pin = http_request_get_query_param(&r->http, "pin");
  const char *did = http_request_get_query_param(&r->http, "did");
  const char *name = http_request_get_query_param(&r->http, "name");
+  if (did && did[0] && !str_is_did(did))
+    return http_request_keyring_response(r, 400, "Invalid DID");
+  if (name && name[0] && !str_is_identity_name(name))
+    return http_request_keyring_response(r, 400, "Invalid Name");
  if (pin)
    keyring_enter_pin(keyring, pin);
  keyring_identity *id = keyring_find_identity_sid(keyring, &r->sid1);
  if (!id)
    return http_request_keyring_response(r, 404, "Identity not found");
-  if (keyring_set_did(id, did ? did : "", name ? name : "") == -1)
+  if (keyring_set_did_name(id, did ? did : "", name ? name : "") == -1)
    return http_request_keyring_response(r, 500, "Could not set identity DID/Name");
  if (keyring_commit(keyring) == -1)
    return http_request_keyring_response(r, 500, "Could not store new identity");
--- a/network_cli.c
+++ b/network_cli.c
@ -787,7 +787,7 @@ static int app_dna_lookup(const struct cli_parsed *parsed, struct cli_context *c
 	    if (strlen((char *)rx.out.payload)<512) {
 	      char sidhex[SID_STRLEN + 1];
 	      char did[DID_MAXSIZE + 1];
-	      char name[64];
+	      char name[ID_NAME_MAXSIZE + 1];
 	      char uri[512];
 	      if ( !parseDnaReply((char *)rx.out.payload, rx.out.payload_length, sidhex, did, name, uri, NULL)
 		|| !str_is_subscriber_id(sidhex)
--- a/overlay_mdp_dnalookup.c
+++ b/overlay_mdp_dnalookup.c
@ -1,6 +1,6 @@
 /*
 Serval DNA MDP lookup service
-Copyright (C) 2016 Flinders University
+Copyright (C) 2016-2017 Flinders University
 Copyright (C) 2010-2015 Serval Project Inc.
 Copyright (C) 2010-2012 Paul Gardner-Stephen
 
@ -28,38 +28,36 @@ DEFINE_BINDING(MDP_PORT_DNALOOKUP, overlay_mdp_service_dnalookup);
 static int overlay_mdp_service_dnalookup(struct internal_mdp_header *header, struct overlay_buffer *payload)
 {
  IN();
-  assert(keyring != NULL);
-  keyring_iterator it;
-  keyring_iterator_start(keyring, &it);
-  char did[64+1];
+  char did[DID_MAXSIZE + 1];
  
-  int pll=ob_remaining(payload);
-  if (pll>64) pll=64;
-  
-  /* get did from the packet */
-  if (pll<1)
+  size_t pll = ob_remaining(payload);
+  if (pll < 1)
    RETURN(WHY("Empty DID in DNA resolution request"));
-  
+  if (pll > sizeof did - 1)
+    pll = sizeof did - 1;
  ob_get_bytes(payload, (unsigned char *)did, pll);
-  did[pll]=0;
+  did[pll] = '\0';
  
-  DEBUG(mdprequests, "MDP_PORT_DNALOOKUP");
+  DEBUGF(mdprequests, "MDP_PORT_DNALOOKUP did=%s", alloca_str_toprint(did));
  
  int results=0;
+  keyring_iterator it;
+  keyring_iterator_start(keyring, &it);
  while(keyring_find_did(&it, did))
    {
+      const char *unpackedDid = (const char *) it.keypair->private_key;
+
      /* package DID and Name into reply (we include the DID because
 	 it could be a wild-card DID search, but the SID is implied 
 	 in the source address of our reply). */
-      if (it.keypair->private_key_len > DID_MAXSIZE) 
+      if (strlen(unpackedDid) > DID_MAXSIZE)
 	/* skip excessively long DID records */
 	continue;
      
-      struct subscriber *subscriber = it.identity->subscriber;
-      const char *unpackedDid = (const char *) it.keypair->private_key;
      const char *name = (const char *)it.keypair->public_key;
-      // URI is sid://SIDHEX/DID
-      strbuf b = strbuf_alloca(SID_STRLEN + DID_MAXSIZE + 10);
+      struct subscriber *subscriber = it.identity->subscriber;
+      // URI is sid://SIDHEX/local/DID
+      strbuf b = strbuf_alloca(SID_STRLEN + DID_MAXSIZE + 20);
      strbuf_puts(b, "sid://");
      strbuf_tohex(b, SID_STRLEN, subscriber->sid.binary);
      strbuf_puts(b, "/local/");
--- a/serval_types.h
+++ b/serval_types.h
@ -145,10 +145,13 @@ int strn_to_identity_t(identity_t *idp, const char *hex, const char **endp);
 typedef uint32_t mdp_port_t;
 #define PRImdp_port_t "#010" PRIx32

-/* DID (phone number)
+/* DID (phone number) and identity name
 */

 #define DID_MINSIZE 5
-#define DID_MAXSIZE 32
+#define DID_MAXSIZE 31
+
+#define ID_NAME_MINSIZE 1
+#define ID_NAME_MAXSIZE 63

 #endif // __SERVAL_DNA__SERVAL_TYPES_H