Fix SSH API

[sjaeckel]

While playing around with the SSH API I realized that as it is now it is incomplete resp. broken.

When reading the RFC4251 Ch. 5 it says

   string

      Arbitrary length binary string.  Strings are allowed to contain
      arbitrary binary data, including null characters and 8-bit
      characters.  ...

which is clearly not possible as it is implemented now.

I think there are two ways this could be handled in the order of preference

1. fix the LTC_SSHDATA_STRING as proposed in the API description of the two c files
2. add a new LTC_SSHDATA_BUFFER as proposed in the header file

The disadvantage of 1. would be that it would break existing applications, but I think we can live with that for the sake of a better API.

Your thoughts?

CC @rmw42 @mkj

[karel-m]

I am for fixing the LTC_SSHDATA

[sjaeckel]

I am for fixing the LTC_SSHDATA

currently writing the documentation on it, should be ready by tonight

[sjaeckel]

Short question, while using the API I thought it'd be useful if the signature of the decoding had a pointer of the length as argument, so we could return the processed length. What do you think?

-int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
+int ssh_decode_sequence_multi(const unsigned char *in, unsigned long *inlen, ...)

Yes, this would differ from the way its inspiration (DER multi) works, but it already differs by the order by which the arguments get passed...

[karel-m]

Following our DER multi pattern is not a good idea, couple of times I was missing the information about the length of actually decoded data.

It is IMO necessary to somehow return how much input data was "consumed" during decoding - either by unsigned long *inlen or via an extra argument.

[sjaeckel]

This would be the behavior I thought about... okay?

-  @param inlen  Length of buffer to decode
+  @param inlen  [in/out] Length of buffer to decode/Remaining bytes after decoding the given sequence

[karel-m]

Well, I would slightly prefer reverse logic: "Actual length of in buffer used for decoding the given sequence" but yours: "Remaining bytes after decoding the given sequence" will work as well.

[sjaeckel]

Well, I would slightly prefer reverse logic: "Actual length of in buffer used for decoding the given sequence" but yours: "Remaining bytes after decoding the given sequence" will work as well.

I thought from a usability context if we'd reverse the logic one has to do

decode(p, &len, multi combination 1);
tot_len -= len;
len = tot_len;
if (some recently decoded thing)
  decode(p, &len, multi variation 1);
else
  decode(p, &len, multi variation 2);
tot_len -= len;
len = tot_len;
decode(p, &len, multi combination 2);

whereas with my proposal one could do

decode(p, &len, multi combination 1);
if (some recently decoded thing)
  decode(p, &len, multi variation 1);
else
  decode(p, &len, multi variation 2);
decode(p, &len, multi combination 2);

[karel-m]

We already have:

/**
   ASN.1 DER Flexi(ble) decoder will decode arbitrary DER packets and create a linked list of the decoded elements.
   @param in      The input buffer
   @param inlen   [in/out] The length of the input buffer and on output the amount of decoded data
   @param out     [out] A pointer to the linked list
   @return CRYPT_OK on success.
*/
int der_decode_sequence_flexi(const unsigned char *in, unsigned long *inlen, ltc_asn1_list **out)

[sjaeckel]

I went with the old way, because I realized that to do what I wanted, it'd have required too many (paradigm) changes.

[sjaeckel]

Does someone have a reason why we shouldn't use size_t where appropriate now? I'd change that then in this PR.

[sjaeckel]

Does someone have a reason why we shouldn't use size_t where appropriate now? I'd change that then in this PR.

okay, one step back, let's just stay with unsigned long for now and change to size_t all at once

[karel-m]

ad size_t we already have #175