Add protective mechanisms to SSL_GetChannelInfo (etc.) to allow applications to detect if a runtime version of NSS cannot supply the expected data (because it is older)

RESOLVED WONTFIX

Status

NSS
Libraries
P1
critical
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: kaie, Unassigned)

Tracking

3.21
3.24

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 14 obsolete attachments)

9.56 KB, patch
Details | Diff | Splinter Review
1.12 KB, patch
kaie
: review+
Details | Diff | Splinter Review
11.33 KB, patch
Details | Diff | Splinter Review
3.36 KB, patch
Details | Diff | Splinter Review
3.44 KB, patch
Details | Diff | Splinter Review
17.98 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Hubert, from the Red Hat quality ensurance team, has discovered a change in NSS 3.21, that looks like a breakage of the NSS ABI stability promises.

This commit:
https://hg.mozilla.org/projects/nss/diff/b858337c4c1b/lib/ssl/sslt.h#l153
changed the length of structure SSLChannelInfo.

A variable of this type is allocated by the application, and used as an output parameter with SSL_GetChannelInfo.

If the application has been compiled against an older version of NSS, older as the version being used at runtime, then function SSL_GetChannelInfo will return failure.

This means, using a newer NSS with an older application will break application functionality.

Hubert suggested, in order to fix that, the function could be changed. Instead of failing, the function could partially fill the result struct.

I don't see a "structure version" in that structure.

It would be good to have some kind of versioning detection. For example, we could distinguish between allowed differences in size, and invalid differences.

(sizeof(SSLChannelInfo) - sizeof(extendedMasterSecretUsed))
could be an allowed size.

I suggest to fix this in a patch release 3.21.1
As Wan-Teh explained on the list, the length parameter is what ensures that this continues to work reliably.  This function is setup specifically so that the struct can be both public AND changeable.  Users of the API are required to check that parameters they are interested in are actually present.  e.g.,

SSLChannelInfo info;
SECStatus rv = SSL_GetChannelInfo(fd, &info, sizeof(info));
if (rv != SECSuccess || 
    offsetof(SSLChannelInfo, sessionIDLength) + sizeof(info.sessionIDLength) > info.length) {
    return SECFailure;
}

I believe that this is correct as implemented.

Comment 2

2 years ago
Martin: you should be able to just verify that info.length == sizeof(info), because that will guarantee that the version you are talking to is the same or newer than yourself.
Yep, that would work.  My version would let you use older versions of NSS than the one you compiled to.  That might be OK if you are built to a recent version (3.21) but only use APIs that are present in an older one (3.14).

Comment 4

2 years ago
yes, verifying that the sizes of fields are sane is necessary only for forwards compatibility, and I don't think we promise that.

OTOH, because this API uses len as a de-facto version number, trying to use application compiled with new NSS when system has old NSS installed won't be caught by linker and can cause uninitialised memory access by the application in effect.

I wonder if we couldn't version this symbol, but just redirect both versions to the same code.

Comment 5

2 years ago
If I understand the scenario you are concerned with, it is something like the following.

The application is compiled against a copy of NSS that has:

struct {
   PRUint32 length;
   PRUint32 X;
   PRUint32 Y;
};

And then run with a copy of NSS that has:
struct {
   PRUint32 length;
   PRUint32 X;
};


When the application calls SSL_GetChannelInfo() it will get back a struct with:

  length = 8;
  X = valid value;
  Y = uninitialized memory

Is that correct?

However, this seems like it's only a problem if the application tries to *read* Y,
in which case it's its responsibility to check .length.
Flags: needinfo?(wtc)

Comment 6

2 years ago
(In reply to Eric Rescorla (:ekr) from comment #5)
> If I understand the scenario you are concerned with, it is something like
> the following.
> 
> The application is compiled against a copy of NSS that has:
> 
> struct {
>    PRUint32 length;
>    PRUint32 X;
>    PRUint32 Y;
> };
> 
> And then run with a copy of NSS that has:
> struct {
>    PRUint32 length;
>    PRUint32 X;
> };
> 
> 
> When the application calls SSL_GetChannelInfo() it will get back a struct
> with:
> 
>   length = 8;
>   X = valid value;
>   Y = uninitialized memory
> 
> Is that correct?

yes

> However, this seems like it's only a problem if the application tries to
> *read* Y,
> in which case it's its responsibility to check .length.

Problem is, I don't see any code in mozilla-central actually doing that. Neither the doc of the struct nor of the function recommend doing that. Length itself is undocumented too.

So even if they have a responsibility to check .length, I don't think we can expect them to.

Thus I'd prefer a link-time error as that would make packaging easier - the rpm itself would simply start depending on newer version of the library.
mozilla-central has other protections in place.  Either the in-tree copy of NSS or this:
https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/configure.in#3510
(Reporter)

Comment 8

2 years ago
We really need to get clarification on the expectations.

From my point of view, we don't promise the scenario described in 5.

As stated in the NSS release notes, we support backwards compatibility:

"NSS 3.21 shared libraries are backward compatible with all older NSS 3.x shared libraries. A program linked with older NSS 3.x shared libraries will work with NSS 3.21 shared libraries without recompiling or relinking. Furthermore, applications that restrict their use of NSS APIs to the functions listed in NSS Public Functions will remain compatible with future versions of the NSS shared libraries."


I've never seen a promise of supporting the other way.

If an application has been compiled against the header files of a "new version B" of NSS, then at runtime, the application binary shall work with "version B" and "version C". I think there isn't requirement for the application to work with older "version A".

If an application is compiled against "version B", it must ensure that at least "version B" (or newer) is being used at runtime, for example by raising packaging requirements to "at least version B of NSS".

If I understand Hubert's statement from comment 5 correctly, Hubert suggests that it should work with "version A". I don't think we ever promised that.


But the complaint from this bug is different.

In the example I mentioned above, the requirement is, the application should work with runtime "version C".


If an application was compiled against NSS 3.20, and the environment is upgraded to version NSS 3.21, the application should continue to work.

However, if the application calls SSL_GetChannelInfo, the implementation of that function in NSS 3.21 will fail.

The implementation contains:

    if (len_given_by_application < len_expected_by_library) { 
	return SECFailure;
    }

In my opinion, this isn't aligned with the compatibiltiy promise.

Using "compiled against NSS 3.20" is used with "NSS 3.21 at runtime", then the above will fail, because of the above code.

IMHO this code is not allowed to fail in this scenario.

IMHO it is the implementation of SSL_GetChannelInfo inside NSS that is required to be lenient, and allow smaller output parameters.
(Reporter)

Comment 9

2 years ago
Created attachment 8717939 [details] [diff] [review]
suggest fix v1
Attachment #8717939 - Flags: review?(martin.thomson)

Comment 10

2 years ago
(In reply to Kai Engert (:kaie) from comment #8)
> We really need to get clarification on the expectations.
> 
> From my point of view, we don't promise the scenario described in 5.
> 
> As stated in the NSS release notes, we support backwards compatibility:
> 
> "NSS 3.21 shared libraries are backward compatible with all older NSS 3.x
> shared libraries. A program linked with older NSS 3.x shared libraries will
> work with NSS 3.21 shared libraries without recompiling or relinking.
> Furthermore, applications that restrict their use of NSS APIs to the
> functions listed in NSS Public Functions will remain compatible with future
> versions of the NSS shared libraries."
> 
> 
> I've never seen a promise of supporting the other way.
> 
> If an application has been compiled against the header files of a "new
> version B" of NSS, then at runtime, the application binary shall work with
> "version B" and "version C". I think there isn't requirement for the
> application to work with older "version A".
> 
> If an application is compiled against "version B", it must ensure that at
> least "version B" (or newer) is being used at runtime, for example by
> raising packaging requirements to "at least version B of NSS".
> 
> If I understand Hubert's statement from comment 5 correctly, Hubert suggests
> that it should work with "version A". I don't think we ever promised that.

No, I'm saying that if the applications wants to continue to work with
"version A" it should do it by explicitly linking to a symbol from "version A",
and not because of the implicit way the method behaves

> In the example I mentioned above, the requirement is, the application should
> work with runtime "version C".

> If an application was compiled against NSS 3.20, and the environment is
> upgraded to version NSS 3.21, the application should continue to work.

and as the code stands now, it will

problem is that it won't stop you from shooting yourself in the foot by downgrading
package when you can't because the application depends on new library behaviour

> However, if the application calls SSL_GetChannelInfo, the implementation of
> that function in NSS 3.21 will fail.
> 
> The implementation contains:
> 
>     if (len_given_by_application < len_expected_by_library) { 
> 	return SECFailure;
>     }

I don't see that code, can you point it out?
 
> In my opinion, this isn't aligned with the compatibiltiy promise.
> 
> Using "compiled against NSS 3.20" is used with "NSS 3.21 at runtime", then
> the above will fail, because of the above code.
> 
> IMHO this code is not allowed to fail in this scenario.
> 
> IMHO it is the implementation of SSL_GetChannelInfo inside NSS that is
> required to be lenient, and allow smaller output parameters.

and it does, handling of output parameters that are larger than expected is the problem.
(Reporter)

Comment 11

2 years ago
Oops. Another confusing detail. Both existing code, and my new code, compare the structure length with sizeof the length variable. It should instead check with sizeof the structure!

The existing check will probably almost never fail, and if the size of the output buffer is at least 4 bytes, the function will overwrite random memory, right?
(Reporter)

Comment 12

2 years ago
Created attachment 8717941 [details] [diff] [review]
patch v2
Attachment #8717939 - Attachment is obsolete: true
Attachment #8717939 - Flags: review?(martin.thomson)
Attachment #8717941 - Flags: review?(martin.thomson)

Comment 13

2 years ago
(In reply to Kai Engert (:kaie) from comment #11)
> The existing check will probably almost never fail, and if the size of the
> output buffer is at least 4 bytes, the function will overwrite random
> memory, right?

No it won't because it's using the smaller of the two values - passed in len or the size of struct as known by library at compile time
(Reporter)

Comment 14

2 years ago
(In reply to Hubert Kario from comment #10)
> > If an application is compiled against "version B", it must ensure that at
> > least "version B" (or newer) is being used at runtime, for example by
> > raising packaging requirements to "at least version B of NSS".
> > 
> > If I understand Hubert's statement from comment 5 correctly, Hubert suggests
> > that it should work with "version A". I don't think we ever promised that.
> 
> No, I'm saying that if the applications wants to continue to work with
> "version A" it should do it by explicitly linking to a symbol from "version
> A",
> and not because of the implicit way the method behaves

Ok, thanks for the clarification. I think we don't need to discuss what an application should do if it is built against version B, but still wants to support version A. Once an environment contains at least one application requiring version B, you cannot go back to version A. This question should be irrelevant for this bug.


> > In the example I mentioned above, the requirement is, the application should
> > work with runtime "version C".
> 
> > If an application was compiled against NSS 3.20, and the environment is
> > upgraded to version NSS 3.21, the application should continue to work.
> 
> and as the code stands now, it will
> 
> problem is that it won't stop you from shooting yourself in the foot by
> downgrading
> package when you can't because the application depends on new library
> behaviour

I think the problem of potentially shooting yourself is offtopic for this bug report. If an application was compiled against version B, using version A is unsupported. There are too many ways in which things can go wrong.


> > However, if the application calls SSL_GetChannelInfo, the implementation of
> > that function in NSS 3.21 will fail.
> > 
> > The implementation contains:
> > 
> >     if (len_given_by_application < len_expected_by_library) { 
> > 	return SECFailure;
> >     }
> 
> I don't see that code, can you point it out?

I'm referring to the following line. In my comment, I had translated the code into a human readable form, to simplify the discussion.
https://hg.mozilla.org/projects/nss/file/507694132f6f/lib/ssl/sslinfo.c#l30


> > In my opinion, this isn't aligned with the compatibiltiy promise.
> > 
> > Using "compiled against NSS 3.20" is used with "NSS 3.21 at runtime", then
> > the above will fail, because of the above code.
> > 
> > IMHO this code is not allowed to fail in this scenario.
> > 
> > IMHO it is the implementation of SSL_GetChannelInfo inside NSS that is
> > required to be lenient, and allow smaller output parameters.
> 
> and it does, handling of output parameters that are larger than expected is
> the problem.

I think you are referring to the bug that I identified in comment 11.

The existing code allows any output structure size that is larger than 4 bytes.

Please see patch v2 for my suggestion to fix the size comparison bug and the compatibility issue.
(Reporter)

Comment 15

2 years ago
(In reply to Hubert Kario from comment #13)
> (In reply to Kai Engert (:kaie) from comment #11)
> > The existing check will probably almost never fail, and if the size of the
> > output buffer is at least 4 bytes, the function will overwrite random
> > memory, right?
> 
> No it won't because it's using the smaller of the two values - passed in len
> or the size of struct as known by library at compile time

Sigh. You're right. I didn't see the use of PR_MIN. Time to rethink everything I said :-(
(Reporter)

Updated

2 years ago
Attachment #8717941 - Attachment is obsolete: true
Attachment #8717941 - Flags: review?(martin.thomson)
Hmm, looks like you found both errors I was writing up already.

I think that there is a simpler solution to the problem of the UMR: memset the provided info using the provided length:

      memset(&inf, 0, sizeof inf);
...
+     memset(info, 0, len);
      memcpy(info, &inf, inf.length);

And then update the documentation to highlight what length is for and how it should be used.  The recommendation that ekr made in comment 2 (check sizeof(SSLChennelInfo) == info.length) is good enough for that.

Comment 17

2 years ago
(In reply to Kai Engert (:kaie) from comment #14)
> (In reply to Hubert Kario from comment #10)
> > > If an application is compiled against "version B", it must ensure that at
> > > least "version B" (or newer) is being used at runtime, for example by
> > > raising packaging requirements to "at least version B of NSS".
> > > 
> > > If I understand Hubert's statement from comment 5 correctly, Hubert suggests
> > > that it should work with "version A". I don't think we ever promised that.
> > 
> > No, I'm saying that if the applications wants to continue to work with
> > "version A" it should do it by explicitly linking to a symbol from "version
> > A",
> > and not because of the implicit way the method behaves
> 
> Ok, thanks for the clarification. I think we don't need to discuss what an
> application should do if it is built against version B, but still wants to
> support version A. Once an environment contains at least one application
> requiring version B, you cannot go back to version A. This question should
> be irrelevant for this bug.

Problem is, that compiling against "version B" doesn't mark the executable as usable
with "version B" or later only. So the environment won't prevent you from downgrading NSS.

(unless the application uses some symbols added in "version B", but that is accidental)

Comment 18

2 years ago
Just to be entirely clear, what I'm referring to is this kind of symbol versioning: https://www.technovelty.org/c/symbol-versions-and-dependencies.html
Comment on attachment 8717939 [details] [diff] [review]
suggest fix v1

Review of attachment 8717939 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/sslinfo.c
@@ +32,5 @@
> +       the extendedMasterSecretUsed attribute, which was added in NSS 3.21 */
> +
> +    if (len == sizeof inf.length) {
> +	haveEMSU = PR_TRUE;
> +    } else if (len == ((sizeof inf.length) - (sizeof inf.extendedMasterSecretUsed))) {

For future reference, sizeof(AStruct) - sizeof(AMember) isn't guaranteed to be the same as sizeof(AStructWithoutAMember).  Padding of structs is possible, particularly when you have types that are smaller than the native platform word or cache-line size.
(In reply to Hubert Kario from comment #18)
> Just to be entirely clear, what I'm referring to is this kind of symbol
> versioning:
> https://www.technovelty.org/c/symbol-versions-and-dependencies.html

Does that work on Windows?  OS X?
(Reporter)

Comment 21

2 years ago
(In reply to Hubert Kario from comment #18)
> Just to be entirely clear, what I'm referring to is this kind of symbol
> versioning:
> https://www.technovelty.org/c/symbol-versions-and-dependencies.html

We're not using versioned shared libraries with NSS.

Comment 22

2 years ago
(In reply to Martin Thomson [:mt:] from comment #20)
> (In reply to Hubert Kario from comment #18)
> > Just to be entirely clear, what I'm referring to is this kind of symbol
> > versioning:
> > https://www.technovelty.org/c/symbol-versions-and-dependencies.html
> 
> Does that work on Windows?  OS X?

honestly, don't know

but can NSS even be installed as a system library on those systems? Even if, I don't think it is nowhere near what you could call "common"

(In reply to Kai Engert (:kaie) from comment #21)
> (In reply to Hubert Kario from comment #18)
> > Just to be entirely clear, what I'm referring to is this kind of symbol
> > versioning:
> > https://www.technovelty.org/c/symbol-versions-and-dependencies.html
> 
> We're not using versioned shared libraries with NSS.

But you do...

$ readelf -a /usr/lib/libssl3.so
   276: 0002fa40   735 FUNC    GLOBAL DEFAULT   12 SSL_GetChannelInfo@@NSS_3.4
...
Version symbols section '.gnu.version' contains 360 entries:
...
(Reporter)

Comment 23

2 years ago
Everyone, I apologize for having created confusion.

This bug report was based on me incorrectly reading the code.

I believe everything here is working as expected, and this bug is actually WONTFIX or INVALID.

The API ensures that the output buffer can contain the output length in bytes, and that's the only requirement it enforces.

    if (!info || len < sizeof inf.length) { 
	PORT_SetError(SEC_ERROR_INVALID_ARGS);
	return SECFailure;
    }

The memset is done on a local variable, using the build time size, which seems safe:

    SSLChannelInfo   inf;
    memset(&inf, 0, sizeof inf);

The code writes at most the size of the output buffer.

  SSL_GetChannelInfo(PRFileDesc *fd, SSLChannelInfo *info, PRUintn len)
  {
    ...
    inf.length = PR_MIN(sizeof inf, len);
    ...
    memcpy(info, &inf, inf.length);

I don't see how this could ever result in uninitialized memory being accessed.

After all, I don't see any bug here. I suggest to resolve as INVALID.


The only issue is the general property of NSS, that downgrading to an older NSS might not be detected, but that's a general property of NSS not using versioned shared libraries and not using versioned symbols.
(Reporter)

Comment 24

2 years ago
(In reply to Hubert Kario from comment #22)
> > 
> > We're not using versioned shared libraries with NSS.
> 
> But you do...
> 
> $ readelf -a /usr/lib/libssl3.so
>    276: 0002fa40   735 FUNC    GLOBAL DEFAULT   12
> SSL_GetChannelInfo@@NSS_3.4
> ...
> Version symbols section '.gnu.version' contains 360 entries:
> ...

That's the version where a symbol got introduced for the first time, right?

I suspect you won't be able to find any function that is listed twice, with different versions.
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID

Comment 25

2 years ago
Hi Kai,

I agree with your marking this bug as invalid. One of us should
submit a patch to document SSLChannelInfo.length.

Hi Hubert,

Thank you for your careful analysis of this change in NSS 3.21.

NSS only supports backward compatibility, defined as follows:

  use application compiled with old NSS when system has new NSS
  installed

NSS doesn't support the scenario you described in comment 4,
which is the other way around:

  use application compiled with new NSS when system has old NSS
  installed

Note that SSL_GetChannelInfo seems to intend to support both
scenarios. In the first scenario (backward compatibility),
there is no issue even if the application doesn't check
SSLChannelInfo.length on return from SSL_GetChannelInfo.
In the second scenario, an application must check
SSLChannelInfo.length to determine which struct members
are valid. This has two problems:

1. This design works in theory. In practice, I believe most
C/C++ programmers would need help to figure out how to do
that because they are probably unfamiliar with |offsetof|.

2. As you pointed out, the need to check SSLChannelInfo.length
on return from SSL_GetChannelInfo is not documented.

I propose that NSS continue to only support backward compatibility,
and we document the need to check SSLChannelInfo.length.
Flags: needinfo?(wtc)

Comment 26

2 years ago
(In reply to Kai Engert (:kaie) from comment #24)
> (In reply to Hubert Kario from comment #22)
> > > 
> > > We're not using versioned shared libraries with NSS.
> > 
> > But you do...
> > 
> > $ readelf -a /usr/lib/libssl3.so
> >    276: 0002fa40   735 FUNC    GLOBAL DEFAULT   12
> > SSL_GetChannelInfo@@NSS_3.4
> > ...
> > Version symbols section '.gnu.version' contains 360 entries:
> > ...
> 
> That's the version where a symbol got introduced for the first time, right?

yes, that's the basic requirement to using versioned symbols - you need to start doing exactly that

> I suspect you won't be able to find any function that is listed twice, with
> different versions.

how is that relevant? Just because all previous changes were binary compatible doesn't mean this change is and a new symbol (but pointing to the same code) can't be introduced.

(In reply to Wan-Teh Chang from comment #25)
> I propose that NSS continue to only support backward compatibility,

And I completely agree. I'm just pointing out that this behaviour is insufficient to prevent application from being incorrectly used with old version.

If you don't want to introduce a versioned symbol, I think the following check is necessary:

    if (len > sizeof inf) { 
	PORT_SetError(SEC_ERROR_INVALID_ARGS);
	return SECFailure;
    }

But I really don't like it as the dependance on new behaviour of the function won't be reflected in linking information of the executable.
(Reporter)

Comment 27

2 years ago
(In reply to Wan-Teh Chang from comment #25)
> I agree with your marking this bug as invalid. One of us should
> submit a patch to document SSLChannelInfo.length.

I agree.


> I propose that NSS continue to only support backward compatibility,
> and we document the need to check SSLChannelInfo.length.

I agree.

I can see that the NSS tool selfserv perform this check:

    SSLChannelInfo    channel;
    result = SSL_GetChannelInfo(fd, &channel, sizeof channel);
    if (result == SECSuccess && 
        channel.length == sizeof channel && 
        ...

I suggest that Firefox PSM and ssl_gtest should be changed to perform that check, too.
See Also: → bug 1247326
(Reporter)

Comment 28

2 years ago
Created attachment 8718052 [details] [diff] [review]
Document expectations around SSL_GetChannelInfo(), fix ssl_gtest
Attachment #8718052 - Flags: review?(wtc)
(Reporter)

Updated

2 years ago
Summary: size of SSL_GetChannelInfo changed in NSS 3.21, breaking ABI compatibility → SSL_GetChannelInfo requires callers to check the length of obtained data

Comment 29

2 years ago
Comment on attachment 8718052 [details] [diff] [review]
Document expectations around SSL_GetChannelInfo(), fix ssl_gtest

Review of attachment 8718052 [details] [diff] [review]:
-----------------------------------------------------------------

SSL_GetPreliminaryChannelInfo which is defined 8 lines below SSL_GetChannelInfo has _exact_ same behaviour but you're not documenting it...

Comment 30

2 years ago
Created attachment 8718788 [details] [diff] [review]
Introduce versioned symbol

The solution to fix this issue properly on platforms that do support symbol versioning.
Attachment #8718788 - Flags: review?(wtc)

Comment 31

2 years ago
Created attachment 8718928 [details] [diff] [review]
Document expectations around SSL_Get* methods, make sure they're not misused in future, fix ssl_gtest

Fix:
 * SSL_GetPreliminaryChannelInfo
 * SSL_GetCipherSuiteInfo
 * SSL_GetChannelInfo

document expected behaviour of user code and the length field in returned structs. Add code preventing their use if user application was compiled against a newer version which increased sizes of those structs. Backwards compatibility remains unchanged.
Attachment #8718788 - Attachment is obsolete: true
Attachment #8718788 - Flags: review?(wtc)
Attachment #8718928 - Flags: review?(wtc)
Comment on attachment 8718788 [details] [diff] [review]
Introduce versioned symbol

If this is built using GCC on windows, does this work or is the versioning stuff ignored.

Comment 33

2 years ago
Comment on attachment 8718928 [details] [diff] [review]
Document expectations around SSL_Get* methods, make sure they're not misused in future, fix ssl_gtest

Review of attachment 8718928 [details] [diff] [review]:
-----------------------------------------------------------------

Hubert: thanks for the patch. We only need to drop forward compatibility
of these three functions. It is not necessary to introduce a versioned
SSL_GetChannelInfo symbol.

Comment 34

2 years ago
Created attachment 8719371 [details] [diff] [review]
Patch v3: Remove forward compatibility support, document SSL_GetChannelInfo etc.

I removed the versioned SSL_GetChannelInfo symbol from
Kai and Hubert's patch. Please review.
Attachment #8718052 - Attachment is obsolete: true
Attachment #8718928 - Attachment is obsolete: true
Attachment #8718052 - Flags: review?(wtc)
Attachment #8718928 - Flags: review?(wtc)
Attachment #8719371 - Flags: superreview?(kaie)
Attachment #8719371 - Flags: review?(martin.thomson)
Comment on attachment 8719371 [details] [diff] [review]
Patch v3: Remove forward compatibility support, document SSL_GetChannelInfo etc.

Review of attachment 8719371 [details] [diff] [review]:
-----------------------------------------------------------------

If .length on all the structs is now obsolete, do we really need to check that (len >= sizeof(info.length)) in all these functions?  That removes the special status of .length completely.
Attachment #8719371 - Flags: review?(martin.thomson) → review+

Comment 36

2 years ago
(In reply to Wan-Teh Chang from comment #33)
> It is not necessary to introduce a versioned
> SSL_GetChannelInfo symbol.

Then why are we checking that the len passed in is big enough to hold length field at all? ChannelInfo never was so small as to not have space for length field.

There are a lot of things that are not necessary, but defence in depth in a security library should be the norm, not the exception. Especially if the library behaviour can very easily lead to CWE-805 in applications that use the library.


Changing the size of the struct in the first place is a risky business as it depends on no library using it as part of a bigger struct (it's not documented that you shouldn't do that) and passing it between it and its users. But I'm not asking you to reverse your decision as I'm aware that this would be messy.

So, please, tell me why you don't want to introduce versioned symbols in addition to other protections.
Flags: needinfo?(wtc)

Comment 37

2 years ago
Hubert,

This was discussed extensively on the NSS Developer's Call on 2-11-16 and your colleagues Kai and Bob can provide more details for you. Versioned symbols are not cross-platform at all, and the approach you've chosen relies on compiler and linker specific extensions. We discussed that if we wished to provide such a guarantee, we would do so in the manner that the code is doing right now, and clearly document it as such.

While your solution is an option, it was an option that was rejected, in favor of providing a consistent cross-platform experience and a consistent cross-platform explanation of how comparability works. These are both technically viable options, and we chose the path with maximum compat.

It is clear the original authors of this function coded it defensively - not just against backwards compat issues but also against misuse of the library by developers. This is no different than the fact that NSS provides return codes to specify invalid arguments were provided - preconditions are validated upon entering, a defensive programming technique you seem to have mistaken for sloppiness, but which was intentional.

In either event, it was decided to prefer a consistent set of guarantees, and this patch fails to do that. Hopefully you can understand this is merely a technical disagreement between valid options with different tradeoffs, and we pursued a path with the least complexity and the best cross-platform set of guarantees for both the NSS maintainers and NSS developers.
Flags: needinfo?(wtc)
Comment on attachment 8719371 [details] [diff] [review]
Patch v3: Remove forward compatibility support, document SSL_GetChannelInfo etc.

Review of attachment 8719371 [details] [diff] [review]:
-----------------------------------------------------------------

This just seems like good defensive practice. If you're going to set info.length you want
to make sure that info is large enough to hold it.

Comment 39

2 years ago
(In reply to Ryan Sleevi from comment #37)
> Hubert,
> 
> This was discussed extensively on the NSS Developer's Call on 2-11-16 and
> your colleagues Kai and Bob can provide more details for you. Versioned
> symbols are not cross-platform at all, and the approach you've chosen relies
> on compiler and linker specific extensions. We discussed that if we wished
> to provide such a guarantee, we would do so in the manner that the code is
> doing right now, and clearly document it as such.
> 
> While your solution is an option, it was an option that was rejected, in
> favor of providing a consistent cross-platform experience and a consistent
> cross-platform explanation of how comparability works. These are both
> technically viable options, and we chose the path with maximum compat.
> 
> It is clear the original authors of this function coded it defensively - not
> just against backwards compat issues but also against misuse of the library
> by developers. This is no different than the fact that NSS provides return
> codes to specify invalid arguments were provided - preconditions are
> validated upon entering, a defensive programming technique you seem to have
> mistaken for sloppiness, but which was intentional.
> 
> In either event, it was decided to prefer a consistent set of guarantees,
> and this patch fails to do that. Hopefully you can understand this is merely
> a technical disagreement between valid options with different tradeoffs, and
> we pursued a path with the least complexity and the best cross-platform set
> of guarantees for both the NSS maintainers and NSS developers.

I never said that versioned symbols should be used alone, both in my previous comments and in the second patch I showed them being used _in addition_ to the mechanisms you want to depend on.

I also didn't say that the function was not programmed defensively. I said that the code that _uses_ the function will be vulnerable.

Comment 40

2 years ago
(In reply to Ryan Sleevi from comment #37)
> Versioned
> symbols are not cross-platform at all, and the approach you've chosen relies
> on compiler and linker specific extensions.

it is cross-platform, exact same mechanism was adopted by FreeBSD: https://people.freebsd.org/~deischen/symver/freebsd_versioning.txt

I simply not verified that it works there as I don't have such a system on hand.

> In either event, it was decided to prefer a consistent set of guarantees,
> and this patch fails to do that.

How exactly it fails to do that? Using code compiled with new NSS but running on system with old NSS is unsupported and will cause the method function to fail when in some future version the ChannelInfo changes again.
The code I provided does the exact same thing but for code compiled _right now_ when it is interfacing to NSS code that will never have those checks.

The guarantee, or rather lack of it, is the same - "unsupported" - it's just that it is enforced on platforms that are most likely to use NSS as a system library.

> Hopefully you can understand this is merely
> a technical disagreement between valid options with different tradeoffs,

The only "tradeoff" I see is that Linux and (possibly) BSD users will have better protections against user mistakes than others.

I see it as catering to lowest common denominator.

Just because Windows users need to live in DLLHell, doesn't mean that others have too.

Comment 41

2 years ago
(In reply to Hubert Kario from comment #40)
> it is cross-platform, exact same mechanism was adopted by FreeBSD:
> https://people.freebsd.org/~deischen/symver/freebsd_versioning.txt
> 

I was specifically referencing that it does not work across all NSS platforms. That is what is meant by "cross-platform" - it works everywhere NSS works.



> The only "tradeoff" I see is that Linux and (possibly) BSD users will have
> better protections against user mistakes than others.
> 
> I see it as catering to lowest common denominator.
> 
> Just because Windows users need to live in DLLHell, doesn't mean that others
> have too.

I can appreciate that you disagree with this. But we did discuss it, and these points were raised. I realize you see it as providing stronger guarantees where possible, but those stronger guarantees can lead to situations where developers expect the linker to sort it out, rather than reading the documentation and understanding those guarantees. Given the added complexity - in adding this code, supporting it, and in trying to maintain future symboling for all symbols (because that is, effectively, what we are talking about - either we do things inconsistently or we do them holistically, and either we accept the complexity or we don't) did not seem warranted.

Hence the statement that we have never provided that guarantee, and the update to the documentation to clarify that.

Comment 42

2 years ago
(In reply to Ryan Sleevi from comment #41)
> (In reply to Hubert Kario from comment #40)
> > it is cross-platform, exact same mechanism was adopted by FreeBSD:
> > https://people.freebsd.org/~deischen/symver/freebsd_versioning.txt
> > 
> 
> I was specifically referencing that it does not work across all NSS
> platforms. That is what is meant by "cross-platform" - it works everywhere
> NSS works.

There are a lot of things that work only on one or very few platforms.

I hope you're not trying to tell me that NSS doesn't use /dev/urandom or /dev/random on UNIX-like platforms...

> > The only "tradeoff" I see is that Linux and (possibly) BSD users will have
> > better protections against user mistakes than others.
> > 
> > I see it as catering to lowest common denominator.
> > 
> > Just because Windows users need to live in DLLHell, doesn't mean that others
> > have too.
> 
> I can appreciate that you disagree with this. But we did discuss it, and
> these points were raised. I realize you see it as providing stronger
> guarantees where possible, but those stronger guarantees can lead to
> situations where developers expect the linker to sort it out, rather than
> reading the documentation and understanding those guarantees.

Developers that code against NSS directly? possible. Users? highly unlikely.

Deploying code compiled on one machine to another one is a common.
Downgrading libraries when debugging issues is also a common _user_ behaviour.

> Given the
> added complexity - in adding this code, supporting it, and in trying to
> maintain future symboling for all symbols (because that is, effectively,
> what we are talking about - either we do things inconsistently or we do them
> holistically, and either we accept the complexity or we don't) did not seem
> warranted.
> 
> Hence the statement that we have never provided that guarantee, and the
> update to the documentation to clarify that.

So you're telling me that something that did happen at most 3 times between 2003-08-16 (NSS 3.10) and 2015-06-17 (3.19.2)[1] - at most since I haven't analysed in detail the changes to SSL3StatisticsStr and NSSCMSSignedDataStr - is something that "doesn't seem warranted"? That is too complex to support?

I really don't know what to say if you consider less than 30 lines of platform specific code to be too much of a burden in over 10 years worth of the library's history. Code that requires no knowledge of NSS internals what so ever, code that reuses mechanisms on which glibc _depends_ on to function properly.

 1 - http://upstream.rosalinux.ru/versions/nss.html
   * NSSCMSSignedDataStr: http://upstream.rosalinux.ru/compat_reports/nss/3.10_to_3.11/abi_compat_report.html#Low_Risk_Problems
   * SSL3StatisticsStr: http://upstream.rosalinux.ru/compat_reports/nss/3.11.9_to_3.12/abi_compat_report.html#Low_Risk_Problems
   * SSLChannelInfoStr: http://upstream.rosalinux.ru/compat_reports/nss/3.12.4_to_3.12.5/abi_compat_report.html#Low_Risk_Problems
(Reporter)

Comment 43

2 years ago
I'm updating the status and the summary of this bug, because there has been agreement to implement some protection mechanisms going forward, like checking that the caller doesn't expect the library to return more information than the library version is able to provide.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: SSL_GetChannelInfo requires callers to check the length of obtained data → Add protective mechanisms to SSL_GetChannelInfo (etc.) to allow applications to detect if a runtime version of NSS cannot supply the expected data (because it is older)
(Reporter)

Comment 44

2 years ago
(In reply to Martin Thomson [:mt:] from comment #35)
> 
> If .length on all the structs is now obsolete, do we really need to check
> that (len >= sizeof(info.length)) in all these functions?  That removes the
> special status of .length completely.

Eric's response was:

(In reply to Eric Rescorla (:ekr) from comment #38)
> This just seems like good defensive practice. If you're going to set
> info.length you want
> to make sure that info is large enough to hold it.

I agree with Eric.

Given that we continue to allow short structures from older applications, and given that we will continue to set the length as an output parameter, it makes sense to me to keep enforcing that minimum structure size.
(Reporter)

Comment 45

2 years ago
Comment on attachment 8719371 [details] [diff] [review]
Patch v3: Remove forward compatibility support, document SSL_GetChannelInfo etc.

I like the additional check this patch adds to the implementation of the APIs.
r=kaie

I don't understand why it's necessary to describe the length field as obsolete, because we continue to set it, and because callers can continue to use it to confirm the number of returned bytes.
Attachment #8719371 - Flags: superreview?(kaie) → superreview+
(Reporter)

Updated

2 years ago
Target Milestone: 3.21.1 → ---
(Reporter)

Updated

2 years ago
Target Milestone: --- → 3.23
(Reporter)

Comment 46

2 years ago
Comment on attachment 8719371 [details] [diff] [review]
Patch v3: Remove forward compatibility support, document SSL_GetChannelInfo etc.

https://hg.mozilla.org/projects/nss/rev/a4beb7ebc5d2
Attachment #8719371 - Flags: checked-in+
(Reporter)

Comment 47

2 years ago
Bob, you said, you'd like to add something like accessor macros.

Do you think we could get that added within the next couple of days, like, by Tuesday, as part of this bug, for the 3.23 release?
Flags: needinfo?(rrelyea)
If we are going to the trouble of making macros for this sort of thing, maybe we should instead stop making additions to SSL_GetChannelInfo and instead make new accessor functions for new information, e.g.:

SECStatus SSL_IsExtendedMasterSecretUsed(sslSocket *ss, PRBool *inUse);

Comment 49

2 years ago
(In reply to Martin Thomson [:mt:] from comment #48)
> If we are going to the trouble of making macros for this sort of thing,
> maybe we should instead stop making additions to SSL_GetChannelInfo and
> instead make new accessor functions for new information, e.g.:
> 
> SECStatus SSL_IsExtendedMasterSecretUsed(sslSocket *ss, PRBool *inUse);

+1

that's what I proposed with my discussion with Bob yesterday

I haven't proposed it originally as I didn't want to break API compatibility, but since not even Firefox uses this new field, I think we can do such a small API break (that being said, the SSL_GetChannelInfo would have to be modified to preserve ABI compatibility)

Comment 50

2 years ago
Yes, I already have most of a patch, which I sent out during our Red hat discussions, so you should already have it. I'll dig it up and attach it to this bug.
Flags: needinfo?(rrelyea)

Comment 51

2 years ago
Created attachment 8722275 [details] [diff] [review]
Add macro to reference new fields in our XXXInfoStructures.

If you need symbol additions, you could also reference an external symbol in the macro to build a dependency with a particular library. That would give you a runtime error if you are running against an older library, but only if you reference the new symbol (just like the case where you reference a new function).

Comment 52

2 years ago
(In reply to Robert Relyea from comment #51)
> Created attachment 8722275 [details] [diff] [review]
> Add macro to reference new fields in our XXXInfoStructures.
> 
> If you need symbol additions, you could also reference an external symbol in
> the macro to build a dependency with a particular library. That would give
> you a runtime error if you are running against an older library, but only if
> you reference the new symbol (just like the case where you reference a new
> function).

I've read the following comment in ssl.def:
>;+# Don't export these DATA symbols on Windows because they don't work right.   
>;+# Use the SEC_ASN1_GET / SEC_ASN1_SUB / SEC_ASN1_XTRN macros to access them.

Will this not interfere with the solution on Windows?

Comment 53

2 years ago
> Will this not interfere with the solution on Windows?

Windows makes the dependency correctly (IIRC), it just doesn't connect the data. Since we are only doing this for the dependency, we should be OK (Also, if we have to we can switch to a dummy function reference on windows).

bob

Comment 54

2 years ago
Created attachment 8724458 [details] [diff] [review]
Restore a comment that was accidentally deleted.

This comment was accidentally deleted in
https://hg.mozilla.org/projects/nss/rev/a4beb7ebc5d2
Attachment #8724458 - Flags: review?(kaie)
(Reporter)

Updated

2 years ago
Attachment #8724458 - Flags: review?(kaie) → review+
(Reporter)

Comment 55

2 years ago
Comment on attachment 8724458 [details] [diff] [review]
Restore a comment that was accidentally deleted.

https://hg.mozilla.org/projects/nss/rev/4e474b9c302c
Attachment #8724458 - Flags: checked-in+
(Reporter)

Comment 56

2 years ago
Comment on attachment 8722275 [details] [diff] [review]
Add macro to reference new fields in our XXXInfoStructures.

Bob, 

- you didn't request a review? on this patch.

- there's a typo in the comment, "exteded" should be -> "extended"

- maybe the instructions in the comments should be more detailed.

  When looking at variable name "reserved_extendedMasterSecretUsed",
  to me, it's not obvious that I must not access the attribute directly.

  I think there should be a clearer comment, and maybe even a stronger
  name of the variable.

- You haven't changed the places where the variable is being used.
  I'm concluding that you expect someone else to make the full patch...
(Reporter)

Comment 57

2 years ago
Created attachment 8729024 [details] [diff] [review]
macro v2 patch

Bob, is this how you envisioned the details?

Wan-Teh (or anyone else), would you like to give feedback?
Attachment #8722275 - Attachment is obsolete: true
Attachment #8729024 - Flags: review?(rrelyea)
Attachment #8729024 - Flags: feedback?(wtc)
Comment on attachment 8729024 [details] [diff] [review]
macro v2 patch

Review of attachment 8729024 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/sslt.h
@@ +169,5 @@
>  
> +/* Use these macros to access the entries in SSLChannelInfo that are named
> + * starting with "UseMacroToAccess_" */
> +#define SSL_CHANNEL_INFO_FIELD_EXISTS(info, field) \
> +   ((info).length >= offsetof(SSLChannelInfo,UseMacroToAccess_##field))

Isn't this more correct?

(info).length >= (offsetof(SSLChannelInfo, UseMacroToAccess_##field) + sizeof((info).UseMacroToAccess_##field))

@@ +175,5 @@
> +#define SSL_CHANNEL_INFO_FIELD_GET(info,field) \
> +   (SSL_CHANNEL_INFO_FIELD_EXISTS(info,field) ? info.UseMacroToAccess_##field : -1)
> +
> +#define SSL_CHANNEL_INFO_FIELD_SET(info,field,value) \
> +   if (SSL_CHANNEL_INFO_FIELD_EXISTS(info,field)) {info.UseMacroToAccess_##field = value;}

I don't think that we need this macro.  (Though it might be a useful construct to include privately in sslinfo.c

Comment 59

2 years ago
Given wtc's remarks in Comment #25, are these macros at all needed?

These macros seem to suffer the same problem as the length field originally did - that is, they exist to allow applications compiled with a newer version of NSS to link against an older version of NSS, which, as noted, is NOT SUPPORTED.

Comment 60

2 years ago
Comment on attachment 8729024 [details] [diff] [review]
macro v2 patch

Review of attachment 8729024 [details] [diff] [review]:
-----------------------------------------------------------------

I concur with both of martin's comments. r+ assuming you at least fix the bug martin identified. (checking for offset+size).
Attachment #8729024 - Flags: review?(rrelyea) → review+
(Reporter)

Comment 61

2 years ago
(In reply to Ryan Sleevi from comment #59)
> Given wtc's remarks in Comment #25, are these macros at all needed?
> 
> These macros seem to suffer the same problem as the length field originally
> did - that is, they exist to allow applications compiled with a newer
> version of NSS to link against an older version of NSS, which, as noted, is
> NOT SUPPORTED.

Even if we don't support it, it might happen accidentally, because we apparently don't have a great way to prevent it from happening across all platforms.
(Reporter)

Updated

2 years ago
Attachment #8729024 - Flags: feedback?(wtc)
(Reporter)

Comment 62

2 years ago
Created attachment 8744294 [details] [diff] [review]
macro v3 patch

Addressed comments in the way Martin requested and that Bob approved. Carrying forward r=rrelyea.
Attachment #8729024 - Attachment is obsolete: true
Attachment #8744294 - Flags: review+
(Reporter)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: 3.23 → 3.24

Comment 64

2 years ago
I remain at the opinion that using macros is a bad idea.

 * It's too easy to just ignore them
 * they don't work nicely with IDEs and type hinting
 * they don't work well (if at all) with refactoring tools

Finally, this code supports running code compiled against new library when dynamically linked against old library!

Didn't we agree that it's something that we do not want and that is something we do not support?
(Reporter)

Comment 65

2 years ago
Hubert, during our meetings, I was under the impression, that you had agreed to the macro approach as a sufficient solution for your purposes.

Comment 66

2 years ago
what? when? I said that I prefer :mt approach, see comment #49

Comment 67

2 years ago
and by macro approach, I assumed that it will be something that will _prevent_ you from running with old versions, not return magic numbers when called on nonexistant fields...
(Reporter)

Comment 68

2 years ago
The difficulty is that we had already released this specific attributed, and because of forward compatibility promises, I don't see a way to remove this particular attribute.

The alternative approach with accessor functions can be used with future additions.

Comment 69

2 years ago
That code is breaking API promise, isn't it?

There's nothing stopping us from making the SSL_GetChannelInfo() support bigger structure than the one in public headers. We can add new accessor function without breaking ABI.
(Reporter)

Comment 70

2 years ago
Hubert, please provide a more detailed explanation of your alternative solution (or an illustrative patch).

Comment 71

2 years ago
Created attachment 8744314 [details] [diff] [review]
Add EMS accessor funcion, remove old field, preserve ABI

as to how we can preserve ABI and add accessor function at the same time, see attached patch

note: I haven't run any tests on it, just checked if it compiles

Comment 72

2 years ago
Alternatively, killing the application (e.g. using abort()) when we detect use of old library, instead of returning the static value of "-1" would also be a solution to misuse of the API.
(Reporter)

Comment 73

2 years ago
(In reply to Hubert Kario from comment #72)
> Alternatively, killing the application (e.g. using abort()) when we detect
> use of old library, instead of returning the static value of "-1" would also
> be a solution to misuse of the API.

I think this is a reasonable suggestion. Reopening, so we can get this done, prior to releasing NSS 3.24.

It will require a change in the definition of the macro interface, as I wasn't able to define a GET() macro, which returns a value on success, and aborts on failure. (Because the NSS assert functions return void, and the strict compiler checking expects a correctly typed value in the failure code path, too).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 74

2 years ago
Created attachment 8745493 [details] [diff] [review]
1247021-macro-abort-v4.patch
Attachment #8745493 - Flags: review?(martin.thomson)
Attachment #8745493 - Flags: feedback?(hkario)
(Reporter)

Comment 75

2 years ago
I've verified that PR_Assert aborts the application, even in optimized builds.
Comment on attachment 8745493 [details] [diff] [review]
1247021-macro-abort-v4.patch

Review of attachment 8745493 [details] [diff] [review]:
-----------------------------------------------------------------

This will break ABI compatibility.  We've shipped extendedMasterSecretUsed already.  Without the name change, I think that you will find that the API is difficult to use and so it won't be.  I tend to agree with Wan-Teh regarding forward compatibility: we don't promise it and so shouldn't spend extra effort in providing it.

I like the _INFO_FIELD_EXISTS macro though, but would prefer to see it ported to the other info structs as well.

::: lib/ssl/sslt.h
@@ +207,5 @@
> +#define SSL_CHANNEL_INFO_FIELD_GET(result, info, field) \
> +   if (!SSL_CHANNEL_INFO_FIELD_EXISTS(info, field)) { \
> +     PR_Assert("Missing attribute in SSLChannelInfo", __FILE__, __LINE__); \
> +   } \
> +   result = info.UseMacroToAccess_##field;

This seems like a poster child for the comma operator.  Does this work?

(SSL_CHANNEL_INFO_FIELD_EXISTS(info, field) ? 0 : PORT_Assert(...)), info.##field
Attachment #8745493 - Flags: review?(martin.thomson)

Comment 77

2 years ago
Comment on attachment 8745493 [details] [diff] [review]
1247021-macro-abort-v4.patch

Review of attachment 8745493 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good
Attachment #8745493 - Flags: feedback?(hkario) → feedback+
(Reporter)

Comment 78

2 years ago
(In reply to Martin Thomson [:mt:] from comment #76)
> 
> This will break ABI compatibility.  We've shipped extendedMasterSecretUsed
> already.  Without the name change, ...

There might be a misunderstanding here.

I don't suggest to keep the old name.

The name change has already been checked in for NSS 3.24, using Bob's r+ from "macro patch v2".

(The name change will break API compatibility (developers will have to change their code to use the new macro when they recompile), but the ABI compatibility should remain, because the variable remains in the same place.)


> I like the _INFO_FIELD_EXISTS macro though, but would prefer to see it
> ported to the other info structs as well.

I'll file a separate bug to request this for the other structs.


> ::: lib/ssl/sslt.h
> @@ +207,5 @@
> > +#define SSL_CHANNEL_INFO_FIELD_GET(result, info, field) \
> > +   if (!SSL_CHANNEL_INFO_FIELD_EXISTS(info, field)) { \
> > +     PR_Assert("Missing attribute in SSLChannelInfo", __FILE__, __LINE__); \
> > +   } \
> > +   result = info.UseMacroToAccess_##field;
> 
> This seems like a poster child for the comma operator.  Does this work?
> 
> (SSL_CHANNEL_INFO_FIELD_EXISTS(info, field) ? 0 : PORT_Assert(...)),
> info.##field


You suggested code for the _EXISTS macro, which doesn't require to access the field.

The comma operator doesn't work for a GET macro that "either returns or aborts", because abort functions return void.

(Also note, PORT_Assert doesn't enforce a runtime abort, only PR_Assert aborts in optimized builds, which is what Hubert suggested.)

I'm open to the idea to abort already as part of the EXISTS check. It would clarify our intention, that we don't "support" linking with an older version of NSS, but rather, we're trying to detect an unsupported scenario.
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 79

2 years ago
Created attachment 8745966 [details] [diff] [review]
1247021-macro-abort-v5.patch

Ensure field exists, abort if it doesn't.
Attachment #8745493 - Attachment is obsolete: true
Attachment #8745966 - Flags: review?(martin.thomson)
(In reply to Kai Engert (:kaie) from comment #78)
> (In reply to Martin Thomson [:mt:] from comment #76)
> > 
> > This will break ABI compatibility.  We've shipped extendedMasterSecretUsed
> > already.  Without the name change, ...
> 
> There might be a misunderstanding here.
> 
> I don't suggest to keep the old name.
> 
> The name change has already been checked in for NSS 3.24, using Bob's r+
> from "macro patch v2".

Oh, I checked, but I hadn't refreshed my tree.  I'm not really happy with that outcome.

In fact, extendedMasterSecretUsed is in NSS 3.23, so it really does break API compatibility (I agree, ABI compat should be OK).
 
> I'm open to the idea to abort already as part of the EXISTS check. It would
> clarify our intention, that we don't "support" linking with an older version
> of NSS, but rather, we're trying to detect an unsupported scenario.

That seems best.  I'm still not liking having to declare a temporary, call a macro, then check the value.  That's three operations where one would do.  I checked, and you can use a comma operator with a void return on the first statement.  That will preserve the API you previously had.
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 81

2 years ago
(In reply to Martin Thomson [:mt:] from comment #80)
> 
> That seems best.  I'm still not liking having to declare a temporary, call a
> macro, then check the value.  That's three operations where one would do.  I
> checked, and you can use a comma operator with a void return on the first
> statement.  That will preserve the API you previously had.


This doesn't compile:

#define TEST_MACRO() \
  ((PR_FALSE) ? ((void)PR_Assert(0,0,0)) : (void))

#define GET_MACRO()
  (TEST_MACRO(), 99)

int main()
{
    int a = GET_MACRO();
}


test-macro.c:5:16: error: expected ‘)’ before ‘?’ token
   (TEST_MACRO(), 99)
                ^
test-macro.c:5:52: error: expected ‘)’ before ‘,’ token
(Reporter)

Comment 82

2 years ago
(In reply to Kai Engert (:kaie) from comment #81)
> #define GET_MACRO()
>   (TEST_MACRO(), 99)


I missed a trailing \ here.

I'll try harder.
(Reporter)

Comment 83

2 years ago
Created attachment 8745981 [details] [diff] [review]
1247021-macro-abort-v6.patch

Ok, this builds!

In the
  (a) ? b : c

expression, using (void) for b didn't work, I got compiler errors like "expected token to the left of :",
but using ((void)0) for b works.
Attachment #8745966 - Attachment is obsolete: true
Attachment #8745966 - Flags: review?(martin.thomson)
Attachment #8745981 - Flags: review?(martin.thomson)
Comment on attachment 8745981 [details] [diff] [review]
1247021-macro-abort-v6.patch

Review of attachment 8745981 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/selfserv/selfserv.c
@@ +412,5 @@
>          result = SSL_GetCipherSuiteInfo(channel.cipherSuite,
>                                          &suite, sizeof suite);
>          if (result == SECSuccess) {
>              const char *EMSUsed = "?";
> +            EMSUsed = SSL_CHANNEL_INFO_FIELD_GET(channel, extendedMasterSecretUsed) ? "Yes": "No";

This is much better.  Does the compiler complain about not initializing EMSUsed?  Because the "?" value is now unused.

::: cmd/tstclnt/tstclnt.c
@@ +118,5 @@
>          result = SSL_GetCipherSuiteInfo(channel.cipherSuite,
>                                          &suite, sizeof suite);
>          if (result == SECSuccess) {
>              const char *EMSUsed = "?";
> +            EMSUsed = SSL_CHANNEL_INFO_FIELD_GET(channel, extendedMasterSecretUsed) ? "Yes": "No";

So much code duplication.

::: lib/ssl/sslinfo.c
@@ -21,5 @@
>  }
>  
>  #define SSL_CHANNEL_INFO_FIELD_SET(info,field,value) \
> -    if (SSL_CHANNEL_INFO_FIELD_EXISTS(info,field)) \
> -        {info.UseMacroToAccess_##field = value;}

This is unnecessary.  We will always have the field in our copy of the struct.

::: lib/ssl/sslt.h
@@ +196,4 @@
>       * This field only has meaning in TLS < 1.3 and will be set to
>       *  PR_FALSE in TLS 1.3.
>       */
>      PRBool UseMacroToAccess_extendedMasterSecretUsed;

As I said before, we should back out the previous patch.  The "UseMacroToAccess_" is unnecessary and breaks NSS 3.23 compat.
Attachment #8745981 - Flags: review?(martin.thomson)
(Reporter)

Comment 85

2 years ago
The renaming was done as a protective measure, to actively push developers into noticing the need to use the macro.

Bob had agreed to renaming.
I don't like the renaming, but I can accept it to achieve the developer awareness.
Martin dislikes the renaming and asks to backout.

If we keep the original name, it's more likely that users will never use the macro.

Hubert, can you agree to keeping the original name, having just the comments that point people to use the macro, and having the macro abort if linked against the old version?

Hubert, since Martin even rejects the renaming, for the sake of API compatibility, your other alternative (completely hiding the attribute), seems out of reach even further.

So, at this time, having the aborting macro with comments seems the best you can get.
Flags: needinfo?(hkario)
(Reporter)

Comment 86

2 years ago
Martin, if we undo the rename to UseMacroToAccess_, and if we don't intend to ever use this style of info struct extension again in the future (but rather, enforce the use of accessor functions for future additions, by hiding new attributs from the exported interface), then it's unnecessary to make the macros general (work with any field). In that case, it would be sufficient to simply use a fixed macro for this particular field.
I don't think that we should stop adding to the structs.  After much thinking about this, that's a perfectly good thing to do.

Comment 88

2 years ago
(In reply to Martin Thomson [:mt:] from comment #84)
> ::: lib/ssl/sslt.h
> @@ +196,4 @@
> >       * This field only has meaning in TLS < 1.3 and will be set to
> >       *  PR_FALSE in TLS 1.3.
> >       */
> >      PRBool UseMacroToAccess_extendedMasterSecretUsed;
> 
> As I said before, we should back out the previous patch.  The
> "UseMacroToAccess_" is unnecessary and breaks NSS 3.23 compat.

Then how do we prevent the application from running with version of NSS that does not support the API the application was compiled against?

In Comment #48 you suggested a function to access them. I've created a patch with that approach: attachment 8744314 [details] [diff] [review].

It's still a new API, not even used by Firefox
Flags: needinfo?(hkario) → needinfo?(martin.thomson)
(Reporter)

Comment 89

2 years ago
Hubert, even if Firefox doesn't use it yet, we've release NSS 3.21, NSS 3.22 and NSS 3.23 with that API, and someone might have already started using it.
Comment on attachment 8744314 [details] [diff] [review]
Add EMS accessor funcion, remove old field, preserve ABI

Review of attachment 8744314 [details] [diff] [review]:
-----------------------------------------------------------------

So this breaks compatibility too.  ABI compat in this case.

It's not terrible; but it is unnecessary.

::: lib/ssl/sslinfo.c
@@ +25,5 @@
> + */
> +typedef struct SSLChannelInfoInternalStr {
> +    SSLChannelInfo inf;
> +    PRBool         extendedMasterSecretUsed;
> +} SSLChannelInfoInternal;

I'm not sure about this and the contortions you go into below.
(In reply to Hubert Kario from comment #88)
> Then how do we prevent the application from running with version of NSS that
> does not support the API the application was compiled against?

Do you have any evidence that this is needed?

All that this bug has done is confirmed that we don't really have a good solution here.  And in the absence of a good solution, I conclude that no solution is better.  That is all.

Currently, we fail the call if an old NSS is used with a newer program.  That's adequate to my mind.  Older versions of NSS will memset(0) any spillover, which is poor, but actually consistent with what the API does in certain circumstances anyway.
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 92

2 years ago
Created attachment 8746016 [details] [diff] [review]
1247021-macro-abort-v8.patch
Attachment #8745981 - Attachment is obsolete: true
(Reporter)

Comment 93

2 years ago
(In reply to Martin Thomson [:mt:] from comment #91)
> 
> Do you have any evidence that this is needed?

The scenario that Hubert is worried about is:
- NSS installed as a system package
- an application is installed that was built (elsewhere) against a newer NSS
- application package didn't explicitly enforce a dependency on NSS >= 3.21
- package management system doesn't know that newer nss is required, 
  and keeps older NSS.
(Reporter)

Comment 94

2 years ago
This is a tough bug.

Anything that strictly enforced Hubert's request would require a change to an API that we have already shipped.

Martin thinks we must not change the API.

In situations like this, when there isn't a consensus, I think a module owner should have the final say.

Bob is a module owner, and Bob has already given r+ to change the attribute and thereby change the API. So let's assume that Bob has carefully weighted the alternatives and let's respect his decision.

Although my personal preference is to keep the API, and in general I share Martin's position, I think it was up to Bob to make the decision, and he already decided to break the API.

Using Bob's decision as a base, I believe that the naming approach, which strongly reminds developers not to use the attribute, combined with the "accessor macro that aborts if necessary" is the best consensus that we can reach at this time.

But whatever we do, we should only break API once. Either we agree on changing the API in the way that Bob has already r+'ed, or, if there's very strong resistance, we should back out, and continue to keep this unfixed, in the hope that we can find a consensus at a later time.

Given Bob's r+ on the API breakage, and given that this attribute is most likely not used very widely yet, my preference is to have this change in a release as early as possible. Because the later we change the API, the more likely it will be that NSS consumers are actually affected and need to adjust their code.

So the question is, can we get an r+ on the patch that I attached, which keeps the renaming that Bob r+'ed, and adds the runtime abort improvement?
(Reporter)

Comment 95

2 years ago
Comment on attachment 8746016 [details] [diff] [review]
1247021-macro-abort-v8.patch

I've addressed most of Martin's comment 84, but I kept the renamed name.
Attachment #8746016 - Flags: review?

Comment 96

2 years ago
(In reply to Martin Thomson [:mt:] from comment #90)
> Comment on attachment 8744314 [details] [diff] [review]
> Add EMS accessor funcion, remove old field, preserve ABI
> 
> Review of attachment 8744314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this breaks compatibility too.  ABI compat in this case.

It does not break ABI compatibility. It will handle both code compiled against 3.21 and 3.19 (equivalent to new code)

> It's not terrible; but it is unnecessary.

So should we revisit the macro substitution of the SSL_GetChannelInfo that versions the exported symbol?

> ::: lib/ssl/sslinfo.c
> @@ +25,5 @@
> > + */
> > +typedef struct SSLChannelInfoInternalStr {
> > +    SSLChannelInfo inf;
> > +    PRBool         extendedMasterSecretUsed;
> > +} SSLChannelInfoInternal;
> 
> I'm not sure about this and the contortions you go into below.

this is the part that guarantees ABI compatibility

C standard guarantees that the sizes of the struct and placement of extendedMasterSecretUsed will be the same for this structure and the 3.21 version of SSLChannelInfo structure, i.e. compilers must not add additional padding between "inf" and "extendedMasterSecretUsed" above what they would add in SSLChannelInfo.
(Reporter)

Comment 97

2 years ago
Created attachment 8746023 [details] [diff] [review]
1247021-macro-abort-v9.patch

slight mistake in v8, this v9 builds.

Comment 98

2 years ago
> So should we revisit the macro substitution of the SSL_GetChannelInfo that versions the exported symbol?

by that I mean, we change SSL_GetChannelInfo in public headers to be a macro that calls newest "version" of the method, something like SSL_GetChannelInfo__3_21

at the same time, we make the library export two symbols, the legacy SSL_GetChannelInfo and the new SSL_GetChannelInfo__3_21, both of which would call the same code. This preserves ABI compatibility, works on Windows and Linux and will prevent users trying to run the library with too old version of the library.
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 99

2 years ago
I'd like to remind you that we had prior consensus, that any solution involving exported symbols can only be accepted if it works universally across all platforms.

I was hoping that we could this issue finalized with a rather small solution that can be completely quickly, like the macro approach that we had used.

Restarting the solution development process will likely require more attention and resources that I think this issue deserves.
(Reporter)

Comment 100

2 years ago
(In reply to Kai Engert (:kaie) from comment #99)
> Restarting the solution development process will likely require more
> attention and resources that I think this issue deserves.

more THAN it deserves

Comment 101

2 years ago
(In reply to Kai Engert (:kaie) from comment #99)
> I'd like to remind you that we had prior consensus, that any solution
> involving exported symbols can only be accepted if it works universally
> across all platforms.

this approach was rejected as "not necessary" rather than "not working"

I'm just reminding Martin that it's not the only possible solution. If the API breaking solution really is unacceptable, we have other options, ones that will require more work, from all people involved.

So while I wouldn't like to start the process from the beginning, I prefer that over not fixing the underlying issue.

Comment 102

2 years ago
(In reply to Hubert Kario from comment #101)
> this approach was rejected as "not necessary" rather than "not working"
> 
> I'm just reminding Martin that it's not the only possible solution. If the
> API breaking solution really is unacceptable, we have other options, ones
> that will require more work, from all people involved.
> 
> So while I wouldn't like to start the process from the beginning, I prefer
> that over not fixing the underlying issue.

It's worth reminding that on our NSS call, multiple people shared Martin's opinion - including Wan-Teh, myself, and, if I recall correctly, Richard. Specifically, that this was a non-issue, and that any change - including the symbol approach - was inappropriately attempting to defend against a situation, at the library level, that is truly a distributor error. I was honestly surprised to see that change land, but I didn't feel it worthwhile to speak up at the time. However, I certainly support the view that this change, as landed, is unnecessary and adds to complexity and readability while providing limited value and causing real API breakages, and would much rather prefer to see the documentation-only approach, which is what was agreed on the call as the right solution.

Comment 103

2 years ago
You are aware that in general this is https://cwe.mitre.org/data/definitions/805.html ?

and yes, distributors do make errors, they are human, just like programmers, that's why we use tools like valgrind, asan, etc. to catch those errors at programming and testing stage, we use dynamic linkers to catch it at runtime

Comment 104

2 years ago
(In reply to Hubert Kario from comment #103)
> You are aware that in general this is
> https://cwe.mitre.org/data/definitions/805.html ?

You are aware that in general this is not, right? This was discussed on the call.

Under the scenario you've raised as a concern, the memory is within the bounds of the structure (as the new structure is larger than the structure being used by the old-library), and is well-defined access. While the memory itself may not be set by the old-library, the new-code can properly defend against this by checking the length.

More explicitly: This is not a bug in the library. While the library officially does not support the pattern you're talking about (meaning it's already a bug in how it's being used, and the number of possible bugs in using a library are infinite, and we cannot defend against those), the library ALREADY offers well-defined semantics even if it is misused in this manner.

> and yes, distributors do make errors, they are human, just like programmers,
> that's why we use tools like valgrind, asan, etc. to catch those errors at
> programming and testing stage, we use dynamic linkers to catch it at runtime

We do not ship libraries with valgrind, asan, ubsan, precisely because these things incur costs that are greater than their value, at scale.
Similarly, we do not introduce overly complicated semantics to defend against unsupported behaviours, because these things incur costs to development and maintenance that are greater than their value, at scale.
I think that Ryan said it all here.  We have adequate safeguards.  SSL_GetXXXInfo will fail if called by an application that was built with a newer version of NSS.
Flags: needinfo?(martin.thomson)

Comment 107

2 years ago
Created attachment 8746476 [details] [diff] [review]
api-preserving-fix.patch

Patch implementing the fix I proposed in comment #98, requires revert of 487a06b963f7
Attachment #8746476 - Flags: review?(martin.thomson)

Comment 108

2 years ago
Created attachment 8746480 [details] [diff] [review]
CipherSuiteInfo-abi-fix.patch

Fix for the new ABI break caused by the changes in SSLCipherSuiteInfoStr in NSS_3_24_BETA7
Attachment #8746480 - Flags: review?(martin.thomson)
Comment on attachment 8746476 [details] [diff] [review]
api-preserving-fix.patch

Review of attachment 8746476 [details] [diff] [review]:
-----------------------------------------------------------------

What is the plan here, add a new function signature every time the struct changes?

__3_21 here, then __3_25 next time.
Attachment #8746476 - Flags: review?(martin.thomson)
Comment on attachment 8746480 [details] [diff] [review]
CipherSuiteInfo-abi-fix.patch

Review of attachment 8746480 [details] [diff] [review]:
-----------------------------------------------------------------

Same question as for the other.

My assessment is that this isn't worthwhile; we'll discuss this later, probably some time during the 3.25 release.
Attachment #8746480 - Flags: review?(martin.thomson)

Comment 111

2 years ago
(In reply to Martin Thomson [:mt:] from comment #109)
> Comment on attachment 8746476 [details] [diff] [review]
> api-preserving-fix.patch
> 
> Review of attachment 8746476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the plan here, add a new function signature every time the struct
> changes?
> 
> __3_21 here, then __3_25 next time.

yes, every time the structure is extended, a new symbol needs to be added. But because the foundation will be already in place, those future patches will be simpler (change 4 lines, add 6).

I also don't expect this to be necessary often. During the last 11 years we would need to have added new symbols 3 times, excluding the above two patches.

and I think we both agree that essentially two pre-processor macros is not something we would call "complex code"

I also volunteer to writing documentation page explaining what and why needs to be done in the future if that (or any other similar) structure is changed again.

(In reply to Martin Thomson [:mt:] from comment #110)
> My assessment is that this isn't worthwhile;

I implore you to reconsider. It's of huge help to all the Linux distributions and it protects other OS users from mistakes too.

> we'll discuss this later,
> probably some time during the 3.25 release.

The 3.24 change haven't shipped yet. Bob accepted API break here, doing the same with CipherSuiteInfo may not be possible (honestly, I'd prefer not to break API if possible).

For now we have all options to fix this available, we won't have after 3.24 ships.
Flags: needinfo?(martin.thomson)
We decided today that since 3.21 through 3.23 are already busted in this fashion, this could wait until 3.25.  And until Bob returns from leave.  We are going to back out the API break in the meantime.
Flags: needinfo?(martin.thomson)

Comment 113

2 years ago
The 3.21 to 3.23 bustage is less of a problem as the added field is just a flag, it can be interpreted to have only two values and correct application code needs to handle both of them. The code will expect one value or something else.

The coming 3.24 change is more worrying because the field is an int (enum, but for practical purposes those are ints).

Imagine that we have following example code:

const char * descriptions[] = {
    "no authentication",    /* ssl_auth_null */
    "static RSA",           /* ssl_auth_rsa_decrypt */
    "DSA authentication",   /* ssl_auth_dsa */
    ...
};

#define n_descriptions (sizeof (descriptions) / sizeof (const char *))
assert(n_descriptions >= ssl_auth_size);

CipherSuiteInfo info;
SECStatus status;
status = SSL_GetCipherSuiteInfo(cipher_ID, &info, sizeof info);
if (status == SECFailure)
   abort();
if (info.authType >= ssl_auth_size)
   abort();
printf("%s\n", descriptions[info.authType]);


despite it being written defensively, it still can cause out of bounds access on descriptions[] - the int is signed, and when uninitialised it may be negative.

So I respectfully disagree that this can wait until 3.25.
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 114

2 years ago
At this time, there is no consensus on what to do.

We shouldn't break API without a consensus.

The "macro v3 patch" breaks API, therefore I will back it out prior to 3.24 release.
This has been agreed to in yesterday's NSS meeting, and we have verbal r=martin.thomson on the backout.

Because we had already shipped the new member SSLChannelInfo.extendedMasterSecretUsed in several NSS releases (3.21, 3.22, 3.23), it seems acceptable to ship it in another release, until we have a consensus amongst NSS module owners and peers.

Regarding the new attribute SSLCipherSuiteInfo.authType, function SSL_GetCipherSuiteInfo has always
  (since 2001, https://hg.mozilla.org/projects/nss/rev/45474dcfbf16#l10.289 )
returned the true size of the returned data in the returned length attribute, therefore applications that check the returned data, for being at least the expected size, are safe.

Given that the returned structs contain a length attribute, it's clear that applications MUST check the returned data, and then they are safe.

The example given in comment 113 doesn't convince me that action prior to the release of NSS 3.24 is required.

We declare that we return integers, and what meanings those integers have. We don't make any promise that those integers are suitable for being used as an array index.

Given that we support old applications used with new libraries, applications already face the risk that at runtime, they'll receive integers they didn't know about at build time. This makes it really bad idea to use such integers as indices for arrays. If it's the other way round, (which we don't support, but is the scenario that Hubert is worried about: new application with old library), I can't imagine how it could be a problem, even if an application made the bad decision to use the integers as an array index. In that scenario, the numbers potientially returned by the old library would be a smaller subset than the newer application might expect. But really, applications should have a switch statement, individually checking for known values with known meaning, not arrays.

We could provide convenience macros, that could be called immediately after having called the information retrieval functions, e.g. like:
  if (SECSuccess == SSL_GetCipherSuiteInfo(suite, &info, sizeof(info))) {
    ABORT_IF_SMALLER(info->length, sizeof(SSLCipherSuiteInfo))
  }
but let's work out the details in the next development cycle.

Comment 115

2 years ago
(In reply to Kai Engert (:kaie) from comment #114)
> Because we had already shipped the new member
> SSLChannelInfo.extendedMasterSecretUsed in several NSS releases (3.21, 3.22,
> 3.23), it seems acceptable to ship it in another release, until we have a
> consensus amongst NSS module owners and peers.

agreed

> Regarding the new attribute SSLCipherSuiteInfo.authType, function
> SSL_GetCipherSuiteInfo has always
>   (since 2001, https://hg.mozilla.org/projects/nss/rev/45474dcfbf16#l10.289 )
> returned the true size of the returned data in the returned length
> attribute, therefore applications that check the returned data, for being at
> least the expected size, are safe.
> 
> Given that the returned structs contain a length attribute, it's clear that
> applications MUST check the returned data, and then they are safe.

to quote the current documentation, the one that will be read by people adding use of this new field:
    /*|length| is obsolete. On return, SSL_GetCipherSuitelInfo sets |length|   
     * to the smaller of the |len| argument and the length of the struct. The   
     * caller may ignore |length|. */

> The example given in comment 113 doesn't convince me that action prior to
> the release of NSS 3.24 is required.

it's not required, the documentation says otherwise, it's correct use of API

> We declare that we return integers, and what meanings those integers have.
> We don't make any promise that those integers are suitable for being used as
> an array index.

and that's why the code has assertions to verify that they are actually usable as array indexes

(ok, maybe assert(ssl_auth_null == 0) is missing, but it wouldn't change anything)

> Given that we support old applications used with new libraries, applications
> already face the risk that at runtime, they'll receive integers they didn't
> know about at build time. This makes it really bad idea to use such integers
> as indices for arrays.

and code does check and handle that (imagine more elaborate handler for that instead of the abort(), but it's unimportant detail)

> If it's the other way round, (which we don't support,
> but is the scenario that Hubert is worried about: new application with old
> library), I can't imagine how it could be a problem, even if an application
> made the bad decision to use the integers as an array index. In that
> scenario, the numbers potientially returned by the old library would be a
> smaller subset than the newer application might expect.

no, the problem is that the field would be completely uninitialised, so it can contain any value. It's a new field that was added in beta7

> But really,
> applications should have a switch statement, individually checking for known
> values with known meaning, not arrays.

it's a continuous list. ABI guarantee says that they will not be removed. If they change (new are added), the code will not compile and then the developer will need to make decision what to do, so again, it's valid and secure use of API as documented.

Comment 116

2 years ago
> > The example given in comment 113 doesn't convince me that action prior to
> > the release of NSS 3.24 is required.
> 
> it's not required, the documentation says otherwise, it's correct use of API

Sorry, I mean that action before 3.24 IS required.

Only the check on |length| is not required, as documentation says
(Reporter)

Comment 117

2 years ago
Created attachment 8747061 [details] [diff] [review]
clarify-api-and-helper-macros-v10.patch
Attachment #8744294 - Attachment is obsolete: true
Attachment #8746016 - Attachment is obsolete: true
Attachment #8746023 - Attachment is obsolete: true
Attachment #8746016 - Flags: review?
Attachment #8747061 - Flags: review?(martin.thomson)
(Reporter)

Comment 118

2 years ago
Comment on attachment 8744294 [details] [diff] [review]
macro v3 patch

this has been backed out:
https://hg.mozilla.org/projects/nss/rev/dd000f32983a
Attachment #8744294 - Flags: review-
Attachment #8744294 - Flags: review+
Attachment #8744294 - Flags: checked-in+

Comment 119

2 years ago
Comment on attachment 8747061 [details] [diff] [review]
clarify-api-and-helper-macros-v10.patch

Review of attachment 8747061 [details] [diff] [review]:
-----------------------------------------------------------------

r-

does not help applications already written against 3.21 API; will remain unnoticed when recompiling against current library

the API is also error prone in use, essentially requiring checking two error values (even if one check is hidden in a macro)
(Reporter)

Comment 120

2 years ago
Despite Hubert's comment 119, I believe patch v10 might be helpful.
It clarifies the API documentation, which I think is worth improving.
During yesterday's meeting, Martin seemed open to this kind of macro, which can be offered as a courtesy for applications.
I didn't intend to provide any automatic protection with patch v10.

Comment 121

2 years ago
(In reply to Kai Engert (:kaie) from comment #120)
> During yesterday's meeting, Martin seemed open to this kind of macro, which
> can be offered as a courtesy for applications.
> I didn't intend to provide any automatic protection with patch v10.

can you explain how approach in attachment 8747061 [details] [diff] [review] is better than the one in attachment 8746476 [details] [diff] [review] for the programmer that uses the library?
(Reporter)

Comment 122

2 years ago
(In reply to Hubert Kario from comment #121)
> can you explain how approach in attachment 8747061 [details] [diff] [review]
> is better than the one in attachment 8746476 [details] [diff] [review] for
> the programmer that uses the library?

At this time I don't want to comment on attachment 8746476 [details] [diff] [review], other than there currently doesn't seem to be a consensus for your suggestion.
Comment on attachment 8747061 [details] [diff] [review]
clarify-api-and-helper-macros-v10.patch

Review of attachment 8747061 [details] [diff] [review]:
-----------------------------------------------------------------

I think that this is the best option that I've seen so far.  Though I have a suggestion for simplification.

::: lib/ssl/sslt.h
@@ +197,5 @@
>  
> +#define NSS_ASSERT_SSLCHANNELINFO_SIZE(returned_data) \
> +  if (returned_data.length < sizeof(SSLChannelInfo)) { \
> +    PR_Assert("Library error, incomplete SSLChannelInfo", __FILE__, __LINE__); \
> +  }

You can simplify this and have a single macro across all the info structs.  Just set this to:

#define NSS_ASSERT_INFOSIZE(info) \
  (info.length < sizeof(info)) && \
    PR_Assert("Library error, info struct incomplete", __FILE__, __LINE__)

I used this form to avoid compilers complaining about a trailing semicolon, you could use  do{} while (0) instead.
Attachment #8747061 - Flags: review?(martin.thomson) → feedback+
(Reporter)

Comment 124

2 years ago
(In reply to Martin Thomson [:mt:] from comment #123)
> You can simplify this and have a single macro across all the info structs. 
> Just set this to:

I agree, having a single macro is sufficient.


> 
> #define NSS_ASSERT_INFOSIZE(info) \
>   (info.length < sizeof(info)) && \
>     PR_Assert("Library error, info struct incomplete", __FILE__, __LINE__)
> 
> I used this form to avoid compilers complaining about a trailing semicolon,
> you could use  do{} while (0) instead.

You cannot have
  bool && void

listsuites.c: In function ‘main’:
listsuites.c:51:41: error: void value not ignored as it ought to be
../../coreconf/rules.mk:388: recipe for target 

If you're worried about double semicolons, the macro can use
  if() action
without a trailing semicolon.
(Reporter)

Comment 125

2 years ago
Created attachment 8747726 [details] [diff] [review]
clarify-api-and-helper-macros-v11.patch

I wonder if we could include this patch in the NSS 3.24 release.

It would allow us to advertize that we encourage the use of the new macro, together with documenting the new attribute.
Attachment #8747061 - Attachment is obsolete: true
Attachment #8747726 - Flags: review?(martin.thomson)

Comment 126

2 years ago
(In reply to Kai Engert (:kaie) from comment #125)
> It would allow us to advertize that we encourage the use of the new macro,
> together with documenting the new attribute.

While I have no objections to adding the macro, I have more reservations about the idea of advertising it as the idiomatic method.

1) It adds more overhead, runtime wise, than just doing an NSS_VersionCheck when the application starts or the library loads
2) Applications can handle this, if we hypothetically supported this case (as shown by the virtual entirity of the Windows API space)
3) It's a packaging error, but comes with runtime costs. We should document it, and note it's available, but I don't think it should be encouraged to sprinkle these around for a situation that should never arise and, when it does, is trivially fixed (see #1)

Comment 127

2 years ago
(In reply to Kai Engert (:kaie) from comment #125)
> Created attachment 8747726 [details] [diff] [review]
> clarify-api-and-helper-macros-v11.patch

r- remains, with the stated reasons
 
> I wonder if we could include this patch in the NSS 3.24 release.

it doesn't hurt, but it also doesn't resolve the issue

Comment 128

2 years ago
(In reply to Ryan Sleevi from comment #126)
> (In reply to Kai Engert (:kaie) from comment #125)
> > It would allow us to advertize that we encourage the use of the new macro,
> > together with documenting the new attribute.
> 
> While I have no objections to adding the macro, I have more reservations
> about the idea of advertising it as the idiomatic method.
> 
> 1) It adds more overhead, runtime wise,

yes, overhead which is encountered on every call (trivial amount, but still...)

> than just doing an NSS_VersionCheck
> when the application starts or the library loads

I'm afraid that it is even less known than the fact that you "must" check length field in returned structs

Could we make it part of NSS_Init?

> 2) Applications can handle this, if we hypothetically supported this case
> (as shown by the virtual entirity of the Windows API space)

problem is, that very few people program in raw Win32 API. I was familiar with Visual Basic, C# and have never encountered this approach. That pattern is completely alien on other OSs.

> 3) It's a packaging error, but comes with runtime costs. We should document
> it, and note it's available, but I don't think it should be encouraged to
> sprinkle these around for a situation that should never arise and, when it
> does, is trivially fixed (see #1)

people make mistakes, to protect against mistakes we use automated tools

it's very easy to forget to update the version dependencies when you add support for new fields in API in your application, mistake that will not be detected without very specialised and extensive testing

Making sure that those mistakes are caught automatically is simply more robust. No matter if it's done by either preventing runtime linking to too old version (by virtue of symbol versioning or adding new accessor functions), automatically inserting aborts (and changing the API so that the code starts to use those aborts) or during library initialisation.

Comment 129

2 years ago
(In reply to Hubert Kario from comment #128)
> yes, overhead which is encountered on every call (trivial amount, but
> still...)

Hubert, you cannot say it's trivial. Surely you understand that's up to the caller to decide.

> I'm afraid that it is even less known than the fact that you "must" check
> length field in returned structs

Now our criteria is "Hubert must understand it" (see below). I'm trying to be polite here, but you are unreasonably setting arbitrary goalposts based on your personal experience, and then moving them, and that is not helpful. It does not feel like you're honestly interested in any solution but your own, and you're ignoring the criticisms of your approach.

You're arguing for the introduction of magic macros. At this point, *nothing* is well known, and all solutions are equal on the table.
 
> > 2) Applications can handle this, if we hypothetically supported this case
> > (as shown by the virtual entirity of the Windows API space)
> 
> problem is, that very few people program in raw Win32 API. I was familiar
> with Visual Basic, C# and have never encountered this approach. That pattern
> is completely alien on other OSs.

Hubert, if the solution has to be well-known to you, then that's a completely unreasonable request. Your statement that "few people program in raw Win32 API" is demonstrably false, as evidenced by the ample development going on. Firefox and Chrome developers, for example, are perfectly familiar with this.

But this is especially onerous when held up against a library-specific "magic macro" - which by definition, fewer people will ever be familiar with, because it will always be specific to NSS. So we have a pattern that will be familiar to some population of NSS developers > 0, and a pattern that will be familiar to 0 NSS developers, and you're arguing for the completely alien one.

> people make mistakes, to protect against mistakes we use automated tools

And we're rewriting NSS in rust... When?

This argument is not a reasonable one, because it can be used to justify any expense. We protect people when the costs are balanced, but when the costs are excessive (... such as the cost of switching everyone to a memory-safe language like Rust), we make informed decisions about the tradeoffs. You've continued to make this argument while ignoring the tradeoffs, and that's not helpful, because it suggests you're unwilling to acknowledge there are tradeoffs.


> Making sure that those mistakes are caught automatically is simply more
> robust. No matter if it's done by either preventing runtime linking to too
> old version (by virtue of symbol versioning or adding new accessor
> functions), automatically inserting aborts (and changing the API so that the
> code starts to use those aborts) or during library initialisation.

You, of course, have missed the option that was previously discussed, which is forcing the linker to do the work. Referencing a version-specific symbol so that the symbol dependency is added to any code, and the loader is unable to resolve the dependency, is another way to force the abort.

But of course, *all* of these solutions must be weighed upon the tradeoffs. To ignore there are tradeoffs is to just be stubborn and ignore the issues at play here. We want a solution that, despite X being unsupported, does not incur unnecessary costs to all developers, especially when the situation of X arising is so minimal.

Comment 130

2 years ago
[insert much longer reply here]

In the mean time:

(In reply to Ryan Sleevi from comment #129)
> We want a solution that, despite X being unsupported, does not
> incur unnecessary costs to all developers, especially when the situation of
> X arising is so minimal.

How the solution proposed in attachment 8746476 [details] [diff] [review] incurs costs to _all_ developers?

Comment 131

2 years ago
(In reply to Hubert Kario from comment #130)
> How the solution proposed in attachment 8746476 [details] [diff] [review]
> incurs costs to _all_ developers?

Hubert, that's been responded to repeatedly. I don't feel there's any variety of ways I can say it that hasn't been said before, on this bug, by myself or Martin.

Comment 132

2 years ago
(In reply to Ryan Sleevi from comment #131)
> (In reply to Hubert Kario from comment #130)
> > How the solution proposed in attachment 8746476 [details] [diff] [review]
> > incurs costs to _all_ developers?
> 
> Hubert, that's been responded to repeatedly. I don't feel there's any
> variety of ways I can say it that hasn't been said before, on this bug, by
> myself or Martin.

If it was, please point me to that comments in particular, I don't see it.

Comment 133

2 years ago
As an NSS module owner, I'm resolving this bug WONTFIX.

We've removed forward compatibility support from the SSL_GetChannelInfo,
SSL_GetPreliminaryChannelInfo, and SSL_GetCipherSuiteInfo functions.
NSS never supports forward compatibility. We don't need to add specialized
protective mechanisms for detecting an older runtime version of NSS.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → WONTFIX

Updated

2 years ago
Attachment #8747726 - Flags: review?(martin.thomson)

Updated

2 years ago
Flags: needinfo?(martin.thomson)
(In reply to Ryan Sleevi from comment #102)
> It's worth reminding that on our NSS call, multiple people shared Martin's
> opinion - including Wan-Teh, myself, and, if I recall correctly, Richard.
> Specifically, that this was a non-issue, and that any change - including the
> symbol approach - was inappropriately attempting to defend against a
> situation, at the library level, that is truly a distributor error.

Is it, though? With my distributor hat on, what's happening is that libssl3 has a SSL_GetChannelInfo symbol associated with the version NSS_3.4. Except it isn't actually ABI compatible with older versions of the same symbol with the same version. I don't think it's a distributor error to expect that a library that keeps symbol versions abide by those versions, and I don't think it's a distributor error to assume that if a program is using version NSS_3.4 of a symbol, it means it can run on any version of NSS > 3.4.
You need to log in before you can comment on or make changes to this bug.