Closed Bug 29152 Opened 24 years ago Closed 20 years ago

Cannot do formsigning - crypto.signText

Categories

(Core Graveyard :: Security: UI, defect, P2)

1.0 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: junruh, Assigned: peterv)

References

()

Details

Attachments

(3 files, 8 obsolete files)

1) Visit http://beavis:86/formsign.html

What is expected: A dialog box should appear asking you to choose one of your 
certs to sign a form, and to click OK. 
What I get: The dialog box does not appear.
targeting m16.  If you reported this bug, and you would like to have it fixed 
sooner, please send me email.  I will see what I can do.  :-)
Target Milestone: M16
Blocks: 13785
Summary: Cannot do formsigning → [feature] Cannot do formsigning
jar, per our conversation this afternoon, I am going to later this bug.  The next 
release of a mozilla based client will not support form signing.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → LATER
OS: Windows NT → All
Target Milestone: M16 → M20
Reopening
Status: RESOLVED → REOPENED
Resolution: LATER → ---
Reassinging to ddrinan
Assignee: dougt → ddrinan
Status: REOPENED → NEW
Target Milestone: M20 → Future
Changing status to later.
Status: NEW → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → LATER
LATER/REMIND resolutions are deprecated, reopening (marked future)...
Status: RESOLVED → REOPENED
Resolution: LATER → ---
nsbeta1
Keywords: nsbeta1
Component: Security: Crypto → Client Library
Product: Browser → PSM
Target Milestone: Future → ---
Version: other → 2.0
Target Milestone: --- → Future
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer
QA Contact: ckritzer → junruh
Blocks: 104166
Severity: normal → enhancement
Should we implement form signing in a backward compatible fashion?

http://developer.netscape.com/docs/manuals/security/sgntxt/sgntxt.htm#method

I was told that IDL interfaces do not support a variable list of argument,
however the old method requires that. For IDL one has to put the variable
argument list into an array, but that is a different interface than was used in
the past and described above.

It is doable, though. I was told that supporting variable argument list
functions in JS requires one to implement the JS function directly in JS code.

So if we want to stay backwards compatible, we need to change the way the
crypto.* object is currently implemented. We need to turn that into a new JS
implementation, and make that forward all calls to PSM interfaces - that way we
can offer the old interface to JS, within that code we re-package the argument
list and forward the call to PSM over IDL.
Assignee: ddrinan → kaie
Status: REOPENED → NEW
Keywords: 4xp
Actually my previous statement seems to be partially wrong. While IDL indeed
does not support variable argument lists, we don't need to replace the current
implementation of the crypto object.

I see the interface definition of crypto.generateCRMFRequest uses an empty
argument list, but the implementation is able to directly access the JS context
is able to extract any number of arguments, as it accesses a variable length
argument API. So what we need seems to be already supported.
Keywords: nsbeta1
When a web site requests form signing, and the user confirms with OK, and the
user is already logged in to the token: Should we force the user to enter the
master password again?
Attached image UI Mock-up for signing window (obsolete) —
I believe that we should require the user to type a password to sign. This is 
not (was not) the behavior in 4.X, but I think it's a better model.  Entering 
the password is very similar to putting your name at the bottom of a contract 
or a receipt.

I'm attaching a mockup of a single window that displays the document and 
provides an input box to "sign" by entering the password.

Attachment #97687 - Attachment is obsolete: true
Attached image UI Mock-up for signing window (obsolete) —
I agree with the prompt for the password.

Instead of showing only the nickname for signing, should we display as many
details of the selected cert as the ssl-client-auth prompt is showing?
Looking at the UI mockup (id=97689) I have a little question: You show an
example of a vacation request which is a text file. Will it also be possible to
show in the signing window an html file? In our application we let people sign
an html file (stored in the form as a hidden field). This way the user can view
and print out the signed document in a nice format. It would be nice if your
signing window would detect if the signed text is html and in this case show it
in html format. 
Roland, my underestanding is that we want to start by supporting text/plain
content only. This is what the old interface offered, passing in a text string.
If the string you pass in is a HTML string, the user will be presented with the
raw HTML source you passed in.
Ok, in this case could you add an an extra option for the caOption parameter,
called for example "askwithouttext"? If caOption has this value you would in
your signing window only show the "sign with" and "enter pin" part and not the
text to be signed.
At the moment in our application we are using the FormSecure applet of
Baltimore. FormSecure in its signing window only asks the user the password
(pin) and to select a certificate (and this only if the user has more than one
certificate to sign with). It does not show the text to be signed. I'm pretty
sure that Capicom of IE works the same way. I really would need the same
functionality in signText.
In my opinion, we should not ask a user to sign something that is not shown.

Wouldn't this be analogue to asking somebody to sign a contract, without
allowing to read the contract?
One interesting question is: What is the semantic meaning of signing?  Is it an
assertion that a section of text was read?  Is it an agreement to abide by a
contract, make payments, etc?  Note that folks can often sign as witnesses,
without reading a document, to simply prove that the document existed and was
signed by another party on a given date.  I can sign email all the time to show
I've really composed it, but that doesn't necessarilly mean that I'm signing to
agree to a contract.  As an example, when I compose a contract, and want to send
it around to be reviewed, can't I sign it so that folks know it was really the
one that I was composing?

I'm showing off my ignorance a bit here, but the question of "how can you sign
without seeing what you are signing" suggests there are some semantics to the
signing that are in dispute.  If signing meant agreement to a contract, then it
sure would make sense to see what you are signing (and not be lured into signing
one thing, when you thought were signing another less significant contract).  If
signing simply is as assertion that the document (or collection of documents)
was assembled, or passed near you, then even if you don't read them, the docs
"did pass nearby."

Could someone try to propose some clarification as to the semantic meaning of
signing a form??

Thanks,

Jim (trying to understand) Roskind
My assumption is: If you sign something, you confirm it comes from you. The
consequences of information coming from you can vary on a broad scale. The
context defines the consequences of the signature. The current interface to add
signing is generic, and therefore I think we should assume the most dangerous
consequences.
For example, the german government recently created a law, that digital
signatures can have the same impact than a written signature.
I've started to implement the feature.
I'm attaching a screenshot of the current state of the new UI.
Attachment #97879 - Attachment is patch: false
Attachment #97879 - Attachment mime type: text/plain → image/png
The screenshot says "site pippki". That's only because I've triggered the dialog
from internal code. If that dialog were triggered from a web site, you'd see a
text like "site www.mozilla.org".
Terry:
 why should we have a pin/pwd field in the dialog?
 You don't have to enter your pin pwd when you sign an s/mime email. Why would
it be different for form signing. People who are worried about somebody else
abusing their keys have the option of setting their password expiration
preferences to 1 minute or "ask everytime", or they can use a removable smartcard.

The issues with adding the pin to the dialog are:
  - Kai's screenshot shows "Master password". If we have a smartcard, then the
text is now wrong. The text may have to change dynamically depending on the cert
selected.
  - We already have a password dialog (separate from the form signing dialog).
In fact, when using a smart card which is not friendly (e.g., some versions of
ActivCard), then that dialog may pop up and request the pin just to show the
certs in the form signing dialog. Then you would have to re-enter the pwd in the
form itself.

Attached image UI implementation v2 / "ask" mode (obsolete) —
I temporarily removed the password from my implementation, until we reached the
final decision.

This screenshot is for the "ask" mode of the signText function, where the user
is aksed to select the signing cert manually.
Attachment #97879 - Attachment is obsolete: true
Attached image UI implementation v2 / "auto" mode (obsolete) —
This is for the "auto" mode, where the application makes the decision which
certificate should be used for signing.
I'm not yet sure whether I like Terry's or Stephane's suggestion better.

Stephane already mentioned one scenario where restricting the password prompt to
the sign confirmation dialog is difficult.

There is another difficulty: When using "auto" mode of the signText interface,
the application needs to iterate over the user's certificate and check which one
has a usable private key, before the dialog is brought up. This is required to
make sure, the application does not choose a cert where the private key is absent.

In my tests with "auto" mode, I was prompted to enter my password before the
dialog came up.

If we indeed want to place the password prompt directly into the dialog, and
make the cert decision in advance, we'd need help from the NSS team to help us
find out whether a cert has a private key, without being logged on. (But I fear
that's not always possible?)
Status: NEW → ASSIGNED
Keywords: nsbeta1nsbeta1+
Target Milestone: Future → 2.4
The primary situation I want to consider is when the user has already typed 
their password (PIN).  This will be the case when they have used other security 
features, such as SSL client authentication or encrypted password storage.

In this case, I think we should require the user to type the password again in 
order to sign a form (or other document).  Since the password will always be 
required for this case, the right thing to do is put the password input on the 
form signing window.
After some meetings and some coding, I'm coming up with a patch.

Some decisions were:
- we'll include the password prompt in the dialog directly
- it's required to enter the password, even if the user is already logged in
- if the user doesn't type the correct password, we'll prompt the user and allow
to retry
- although the old interface offered two modes "auto" and "ask" for certificate
selection, we don't see a disadvantage, but a real advantage, to always behave
as if "ask" were specified (show the user a list of certs to select from)

I'm asking for your comments on how work should continue, since the following is
yet unaddressed:
- we should probably display an error message if the user enters a wrong
password on first attempt, before we prompt the user again. Do you agree in
general? Do you agree that it must be a separate message like: "Wrong password
entered. You may try again. [ok]"
- the patch has a problem if the user tries to sign using an invalid (e.g.
expired) cert. Currently we crash (null dereference, signed data object can not
be created, no null check yet). I probably need to find out whether the cert is
valid before trying to sign. Should I do that check before bringing the dialog
or only at the time the user selected a cert?


In addition, all my tests so far have been of rather theoretical nature.
In the JavaScript that executed the crypto.signText call, I retrieved the string
successfully and dumped it. Next I made a manual test using openssl to look at
the object. It looks good.

It would *really* help to know somebody, who actually uses Communicator's
crypto.signText feature to get some feedback.

The first step that is required is: Set up a web site page that calls signText
on a form submit, do the required magic (which requires hooking into form submit
and call signText manually) and checking on the server side, whether the
expected content arrives.

The second step, which is much harder, would be: Enhance the server site to
automatically give feedback, whether the received signed data is what is
expected. This probably requires dynamic PKCS#7 handling on the web server.

I would like to ask for help to create the test sites, or to find people who
could test with their existing deployment.
Another detail: It was suggested the UI should show the name of the token where
the selected certificate is stored on, so that the user is able to know which
password to enter.

I decided to use a standard password prompt text, but extend the "cert details"
text that is shown for the selected cert. It now includes the token name.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #98166 - Attachment is obsolete: true
Attachment #98165 - Attachment is obsolete: true
Comment on attachment 97879 [details]
ScreenShot of implementation attempt

The UI used by the latest patch looks pretty much like this screenshot. The
differences are the enhanced wordings and cert detail text.
Attachment #97879 - Attachment is obsolete: false
Attachment #97689 - Flags: review+
Attachment #97689 - Attachment is obsolete: true
Attachment #97689 - Flags: review+
Keywords: nsbeta1+nsbeta1-
Target Milestone: 2.4 → Future
Attached patch Patch v1 (merged) (obsolete) — Splinter Review
Attachment #99992 - Attachment is obsolete: true
QA Contact: junruh → bmartin
will the patch be checked-in?


should bug 139258 be marked as duplicate of this one?
Summary: [feature] Cannot do formsigning → [feature] Cannot do formsigning - crypto.signText
*** Bug 139258 has been marked as a duplicate of this bug. ***
Some info mentioned in the duplicate bug:

"There is a solution to this problem from Lfern  (http://secclab.mozdev.org) or
(http://www.sourceforge.net/projects/formx/). It would be good if these could be
integrated into Mozilla, so that no downloading of XPI package would be needed."
Summary: [feature] Cannot do formsigning - crypto.signText → Cannot do formsigning - crypto.signText
we currently looking into developing a webmail that can do s/mime signing and
decrypting. that's way I needed signText
It would be *really* useful to me if this was integrated in time for 1.4 branch.
Has anyone tested the patch?
should nsenterprise keyword be added here (as in bug 139258) ?

requesting blocking 1.4 (as there is a patch it should be easy to test it and
check it in)
Flags: blocking1.4?
Blocks: 48444
For what it's worth, stability enhanced Linux, 
Mac OSX and Windows builds based on Mozilla 1.3.1, 
that also contain the patch
from this bug, are available at http://wamcom.org
mozilla1.4 shipped. unsetting blocking1.4 request.
Flags: blocking1.4?
Could someone elivate this bug from P3?  This is causing real-world blocking,
limiting Mozilla's usefulness.  For an example, see:

http://www.openca.org/openca/docs/online/apas02.html#id2832250

Currently I cannot use Mozilla, or Mozilla-derived browsers, for Cert requests
with any OpenCA-based CAs.
I changed the priority, but someone will need to reassign this to themself.
Apparently there is a patch in comment #41
Severity: enhancement → normal
Priority: P3 → P2
Flags: blocking1.6a?
We're not going to block the release for a feature without reviews. If this gets
appropriate reviews in time for 1.6a, please request approval at the patch. 
Flags: blocking1.6a? → blocking1.6a-
regarding comment #43
Mozilla supports pkcs #10 which is the industry standard for cert requests. To
my knowledge, open CAs also support cert requests of that type.
signText is not needed for cert issuance.
regarding comment #46
1. OpenCA still supports and Mozilla still can use SPKAC
2. OpenCA includes the possibility to change request data and then to approve a
request. If you want to protect approved requests against manipulation then it
is a good idea to sign the request and the additional infos. We can do this with
old Netscapes with the javascript object crypto, on Internet Explorer we can do
it too but a default Mozilla is not usable as a RA operator browser in High Risk
environments because the browser cannot sign data. This is what comment #43
means. The result is a recommendation for OpenCA on Unix and the operator should
use Windows 200x with IE.

We (OpenCA guys) are a little bit frustrated about the time it takes to fix this
issue - 3 years and more than 1 year after the first patch :(
>We're not going to block the release for a feature without reviews. If this gets
>appropriate reviews in time for 1.6a, please request approval at the patch. 

Asa--

What help would be required to get the wamcom patch into the tree?  I'm willing
to  do something on this, if you will tell me what you need done.

Thanks,
Rob
Once Upon A Time: rthorne@netscape.com
I'll create an updated patch.
Hello.

There are a number of services provided by the Government and Regional
Administracion in Spain that require some kind of information to be signed with
the Certificate provided by the Spanish Official Certification Agency
(www.fnmt.es). These services include things like tax declarations, the
adquisition of public debt or the renovation of the Certificate itself.

All these things do not work under Mozilla and, in some cases, the problem is
solved by using the old Netscape 4.79. It seems to me that the problem is
related to the subject discussed in this bug, isn't it?

So, it is very important for Spanish users (perhaps from other countries also)
to see this feature implemented, in the meanwhile these important services of
e-government are not usable under Mozilla.

It is expected that this would be included in future releases?
I think it's a good idea to get this patch in.
Unfortunately I've got very little time.
If someone adjusts the patch to work with current Mozilla, both Seamoney and
Firebird, I'll r= it.
Thanks.
Assignee: kaie → peterv
Status: ASSIGNED → NEW
In seems that the bug is now assigned to Peter Van der Beken.

Peter, I've got a lot of experience using signtext on netscape 4.x.
So, if you like, I could be one of the testers for your new patches
to solve this signtext bug.

By the way, Peter, do you belive that this bug will be solved someday?
Yes, I am now working on this bug so I expect it will be fixed someday. If
anyone wants to help, they can write a testcase for this bug and attach it.
Status: NEW → ASSIGNED
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #115282 - Attachment is obsolete: true
Attachment #142451 - Flags: review?(kaie)
Hello, Peter. I think that you have included a new patch. I would like to
test it using my certificate against pages of the Spanish Administration but
I am just an user, not a depeloper and have no knowledge of programming. Is
there any easy way I can install the patch and test it (at the moment I am using
RPM-installed Mozilla 1.6)?. Thanks a lot for your work.
Peter -- what versions have you tried the patch against?  Would it work against
the tag for 1.6 final?
This patch is against trunk, no idea if it will work with the 1.6 tag (it can of
course be ported). I can't provide binaries with the patch right now, maybe
someone else can build them but please post the platforms you need this for so
that we know what to provide. I haven't really been able to test this patch in a
real-world testcase yet (and no one provided one).
(In reply to comment #57)
> This patch is against trunk, no idea if it will work with the 1.6 tag (it can of
> course be ported). I can't provide binaries with the patch right now, maybe
> someone else can build them but please post the platforms you need this for so
> that we know what to provide. I haven't really been able to test this patch in a
> real-world testcase yet (and no one provided one).

Peter, we at spanish FNMT-RCM manage a PKI for the Spanish government and we are
very interested in helping you. Unfortunately, we cannot provide you a
"real-world" test certificate (you are not an Spanish citizen I assume). But I
can send you a "development" cert, and an  html page, and if you grab the output
with any tool (I can show you how to do it with netcat) and send it to me, I can
test the result against our development framework (that is supposed to be
identical to the production one ;-)). If you are interested, let me know and
I´ll send you the tools.

Or, if you prefer, if you send us Microsoft/Linux/Solaris binaries, we could
test it for you.

Regards
Alvaro, if you want to try something out in general, you might want to test the
binaries available at http://wamcom.org

For testing with the most recent versions, please wait some more. Now that Peter
has provided a patch I'm optimistic it won't take much longer to land the patch.

I'm reviewing the patch today.
You can test the patched version at

https://ra.hu-berlin.de/cgi-bin/hudca1/internet_en/pki?cmd=test_cert

If you use the sign button then you have to sign some data and the software
validates the signature. The result is usually a signature error because the
used certificate is not from our PKI. I hope you now how to create a cert for a
Mozilla.
I've sent Álvaro a capture of a signed form submit using his development
certificate, hopefully he'll be able to verify it :-). I'll also attach a new
patch soon that deals better with various error codes. It's unfortunate that
signText only returns an error string on failure but doesn't throw n exception,
it doesn't really fit in the new world but I'll keep that behaviour for
backwards compatibility. If anyone disagrees with that, speak up.
Comment on attachment 142451 [details] [diff] [review]
v1.1

I compared this patch to the patch I created earlier, so I could understand
what you changed.

I think you did a very good job, thanks for cleaning up the patch and merging
it with the trunk.

I'm currently short on build environment resources, so I can't quickly build on
my own. However, I will help to test once we have a nightly build having the
patch.

r=kaie
Attachment #142451 - Flags: review?(kaie) → review+
Here is a simple test script:

http://www.t8m.info/verify.php

It verifies only the correctness of the signature and not the authenticity of
the signer's certificate so you can use any personal certificate you have.

Please be patient with the script it's sometimes overloaded as it's hosted on a
 free webhosting.
Peter, no problem, just attach the new patch. I'll compare your new patch
against the reviewed patch to see what you changed.
Hmm, it seems the page at http://www.t8m.info/verify.php uses different code for
Moz or for NS4, which means it would actually be ok to throw an exception. I
don't really know what to do about the errors, I just removed all the error
returns :-/.

I also get the following:

Verification failed!
error:21071065:PKCS7 routines:func(113):reason(101)
error:21075069:PKCS7 routines:func(117):reason(105)

so there seems to be some error.
I made a couple of months ago several tests of Wancom against some pages of
the Spanish e-Administration. I did a report that you can find in:
https://listas.hispalinux.es/pipermail/sl-administracion/2004-January/003088.html

Sorry it is in Spanish. Resuming its contents:

- FNMT (www.ceres.fnmt.es, the agency that provides the certificates):

I've been able to update my personal data signing it using Wamcom. This is not
possible with regular Mozilla. I think that it should be also possible to renew
the certificate using Wamcom (I cannot test until my certificate is close to
expiration).

- Tesoro Público (www.tesoro.es, to buy and sell public debt).

It is necessary to fill and sign a form to register in the system. It is not
possible with regular Mozilla. With wamcom, after filling the form and press
"sign", a blank page (the form information should be expected) appears to be
signed. If signed, the system returns an "non-valid hash value" error.

There are other public utilities that do not work, but with no difference
between regular Mozilla and wamcom. I think that the problem in these cases is
not crypto.signtext but wrong javascript code.

I can provide a translation of my report if you think it may be useful (probably
not).

On the other hand, the test pages indicated by Michael Bell and Tom Mraz seem to
work with Wancom and my FNMT certificate.
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #142451 - Attachment is obsolete: true
Comment on attachment 142676 [details] [diff] [review]
v1.2

Small changes to nsCrypto to always return NS_OK and signal errors in the
result string to be compatible with NS4.x. I wonder if we should change the
text in the dialog btw. Right now it says "To confirm you agree to sign this
text message using your selected certificate, please confirm by entering the
certificate password:", but one really needs to enter the Master Password
afaict. Should I change "the certificate password" by "Master Password"?
Attachment #142676 - Flags: review?(kaie)
Comment on attachment 142676 [details] [diff] [review]
v1.2

r=kaie

I agree with your proposed string change, no need to attach a new patch.

No need to get superreview, moa=kaie
Attachment #142676 - Flags: review?(kaie) → review+
CC'ing John, to make sure he is aware of this change.
Actually, if one is signing with a cert that is on a hardware device, it will be
the token's password, not the master password.  We should probably look up the
name of the token and include it in the prompt.
Right, however, the password on the token is also called master password in PSM.
In the user interface displayed we give the name of the token the selected
certificate is stored, so we should be fine.

I propose, because of the complexity of the patch, we land the patch first, and
do all the minor tweaks in add on patches.
Tom raised an interesting problem (by private mail) which would certainly
explain why it doesn't work. The hash should be computed based on the string
encoded with the page's encoding, currently we use the UTF-16 encoding. It might
be hard to get at the page's encoding, I want to try first though.
If it would be too hard to get the actual page's encoding I'd suggest to use
UTF-8. It wouldn't be exactly compatible with NS4 but it would allow to use the
crypto.signText internationally.

I've retried the NS4 with various encodings and it seems to me that only
ISO-8859-1 works. So actually for maximum compatibility it should use this
encoding, but then it will be usable only in western countries.
Thanks Kai, we´ve tested Wamcom and it works as good as Netscape 4.7x did. In
fact, Mozilla works fine right now in order to obtain a certificate and use it
for certain purposes (ssl client authentication and this things). The problem is
with the applications that require signing (certificate renewal and personal
data modification).

But I think there are much more Mozilla users than Wancom ones, and I don´t
think it is a good thing for us to force our users to use a particular vwersion
of a certain browser ... we prefer that browsers implement form signing and let
the users decide :-).
I think that UTF-16 encoding is a good fit.
Its Java equivalent is called "UnicodeLittleUnmarked" right?

This is what MS CAPICOM does when it signs String.

The problem with not knowing the character set of text in form fields is a tough
one. I propose to just use UTF-16 encoding for now.
It should be included in the release notes (or other visible document, or a
HOWTO...).

I am willing to produce an initial version of a howto document ...
I would prefer UTF-16 too because otherwise there would be a big problem with
the i18n in web-based projects. Our project (OpenCA) tranlates web-pages too and
this requires support for UTF-16 if you only want to support one characterset.
Otherwise we get serious problems with non-european users which means users from
Asia cannot use form signing.
Some good new: I've managed to get http://www.t8m.info/verify.php to work by
using the page encoding (which is ISO-8859-2).
I'm not against using UTF-16, but the question is how useful that would be as it
wouldn't be compatible with NS4.x. On the other hand, using a fixed encoding
could make the result more predictable, presumably you already know the form
encoding and you can just convert the form data to the right encoding before
computing the hash on the server. Note that it is much harder in that code to
get the form encoding instead of the page encoding, and they can be different!
UTF-8 has the advantage of being somewhat compatible with NS4.x (though not much).
I'm leaning towards UTF-8 right now, since it avoids the issue of endianness
(the current patch ignores it which is a bug imo).

Tom: do you think you could make a page that allows me to set the encoding of
the data used to calculate the hash? It would make testing easier, whatever
encoding we choose.
Done. You can specify encoding of the data in the input dialog box. The script
recodes the data which it gets from the form in utf-8 (i've changed the encoding
of the page) to the specified encoding before verifying.
:-) It works with our application, THANK YOU.
My point is that we could add some 'options' to the signText functions so as to
fullfill every need. We could add:

1. Support for choosing the encoding type when calculating the hash
2. Support for non-detached signatures where data is added within the PKCS#7
   structure
3. Adding the armour to the computed data '-----BEGIN PKCS7-----' and
   '-----END PKCS7-----' so as to be directly feadable to any extenal software
   for signature verification/manipulation

Finally, I would push for the UTF-8 encoding as default.
That would be a followup bug, and a different method imho. signText already
takes a variable number of arguments to select the trusted CAs.
Whatever the encoding we choose for signText the JS code wouldn't need to
change, just the server-side form processor and I want to keep it that way. So I
propose you file a follow-up RFE bug and CC me. I can't promise I'll have the
time to solve it.
I agree with Peter Van der Beken.
Please, Massimiliano, don't ask for new features.
We've been waiting four years for this bug to be solved and
it seems that it will be solved shortly (thanks, Peter).
In my opinion, it's not a good idea to ask for new features.
We just need signText work like it worked with netscape 4.xx.
I agree with you, so the patch makes its way as it is and we'll eventually open
a follow-up bug.
Attached patch v1.3 (final)Splinter Review
This is the last revision (I promise ;-)), calculate the hash using the
document charset. I've tested with two different examples and it finally works
correctly.
Attachment #142676 - Attachment is obsolete: true
Attachment #142830 - Flags: review?(kaie)
Comment on attachment 142830 [details] [diff] [review]
v1.3 (final)

r=kaie

Since you are a superreviewer, I don't require you to get another superreview.
Please decide for yourself whether you want one.

Thanks for your work!
Attachment #142830 - Flags: review?(kaie) → review+
Comment on attachment 142830 [details] [diff] [review]
v1.3 (final)

- In dom/public/idl/base/nsIDOMCrypto.idl:

-  DOMString		     signText(/* ... */);
+  DOMString		     signText(in DOMString stringToSign,
+				      in DOMString caOption /* ... */);

I don't like the the fact that you're making this method callable from JS only.
Shouldn't we have a version of this that's usable from other languages as well?

- In nsCrypto::SignText():

+  PRUnichar **certNicknameList = (PRUnichar
**)nsMemory::Alloc(sizeof(PRUnichar *) * nicknames->numnicknames);
+  PRUnichar **certDetailsList = (PRUnichar **)nsMemory::Alloc(sizeof(PRUnichar
*) * nicknames->numnicknames);

Hmm, you use nsAutoArrayPtr earlier in this method, it'd be cool if you could
do the same here, but if you can't |new| what you alloc into this array, I
guess you can't...

+      if (NS_SUCCEEDED(rv)) {
+	 certNicknameList[certsToUse] = ToNewUnicode(nickWithSerial);
+	 certDetailsList[certsToUse] = ToNewUnicode(details);
+      }
+      else {
+	 certNicknameList[certsToUse] = nsnull;
+	 certDetailsList[certsToUse] = nsnull;
+      }

No error checking here?

+  PRInt32 k;
+  for (k = 0; k < certsToUse; ++k) {
+    if (certNicknameList[k]) {
+      nsMemory::Free(certNicknameList[k]);
+      nsMemory::Free(certDetailsList[k]);
+    }
+  }

This'll crash if we're OOM and ToNewUnicode() fails above. And maybe make this
use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY()?


+  nsMemory::Free(certNicknameList);
+  nsMemory::Free(certDetailsList);

This'll crash on OOM too.

...
+      buffer = ToNewUTF8String(aStringToSign);
+    }
+  }
+
+  HASHContext *hc = HASH_Create(HASH_AlgSHA1);
+  if (!hc) {
+    nsMemory::Free(buffer);

More crashes on OOM.

Any reason not to use an NS_ConvertUTF16toUTF8() here and use its buffer when
not getting a buffer from the encoder? Geez, we really need an nsAutoPtr like
thing that uses nsMemory too, and not just new/delete.

sr=jst with that fixed.
Attachment #142830 - Flags: superreview+
(In reply to comment #87)
> (From update of attachment 142830 [details] [diff] [review])
> - In dom/public/idl/base/nsIDOMCrypto.idl:
> 
> -  DOMString		     signText(/* ... */);
> +  DOMString		     signText(in DOMString stringToSign,
> +				      in DOMString caOption /* ... */);
> 
> I don't like the the fact that you're making this method callable from JS only.
> Shouldn't we have a version of this that's usable from other languages as well?

I don't feel like breaking backwards compatibility for JS users and it's not my
fault that the Ns4.x API sucks. There's a RFE bug filed for a better API (bug
236335).

> - In nsCrypto::SignText():
> 
> +  PRUnichar **certNicknameList = (PRUnichar
> **)nsMemory::Alloc(sizeof(PRUnichar *) * nicknames->numnicknames);
> +  PRUnichar **certDetailsList = (PRUnichar **)nsMemory::Alloc(sizeof(PRUnichar
> *) * nicknames->numnicknames);
> 
> Hmm, you use nsAutoArrayPtr earlier in this method, it'd be cool if you could
> do the same here, but if you can't |new| what you alloc into this array, I
> guess you can't...

Hmm, I guess I could. I'll add an OOM check too.

> +  PRInt32 k;
> +  for (k = 0; k < certsToUse; ++k) {
> +    if (certNicknameList[k]) {
> +      nsMemory::Free(certNicknameList[k]);
> +      nsMemory::Free(certDetailsList[k]);
> +    }
> +  }
> 
> This'll crash if we're OOM and ToNewUnicode() fails above. And maybe make this
> use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY()?

Well, that'll loop twice ;-). I'll do the nsAutoArrayPtr dance and add a |if
(certDetailsList[k])|. Note that NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY doesn't
allow one to null-check anyway.

> +      buffer = ToNewUTF8String(aStringToSign);
> +    }
> +  }
> +
> +  HASHContext *hc = HASH_Create(HASH_AlgSHA1);
> +  if (!hc) {
> +    nsMemory::Free(buffer);
> 
> More crashes on OOM.

Memory is cheap ;-).

> Any reason not to use an NS_ConvertUTF16toUTF8() here and use its buffer when
> not getting a buffer from the encoder? Geez, we really need an nsAutoPtr like
> thing that uses nsMemory too, and not just new/delete.

Oh yes :-). I don't see how I could easily use an NS_ConvertUTF16toUTF8 there,
since the ToNewUTF8String is inside an else and I need that buffer outside the
else block.
> I don't feel like breaking backwards compatibility for JS users and it's not my
> fault that the Ns4.x API sucks. There's a RFE bug filed for a better API (bug
> 236335).

Ok, fair enough. I wasn't suggesting we'd break the JS api tho, just provide a
non-scriptable (or scriptable, if that's easier) way to do the same, in paralell
to the JS API.

> > Any reason not to use an NS_ConvertUTF16toUTF8() here and use its buffer when
> > not getting a buffer from the encoder? Geez, we really need an nsAutoPtr like
> > thing that uses nsMemory too, and not just new/delete.
> 
> Oh yes :-). I don't see how I could easily use an NS_ConvertUTF16toUTF8 there,
> since the ToNewUTF8String is inside an else and I need that buffer outside the
> else block.
> 

Put the NS_ConvertUTF16toUTF8 object outside the if/else, and free if buffer !=
myutf8string.get(). :-) Either way, I'm cool with whichever approach.
> Put the NS_ConvertUTF16toUTF8 object outside the if/else, and free if buffer !=
> myutf8string.get(). :-) Either way, I'm cool with whichever approach.

I'll just ignore this, there is no way to do that without converting twice since
the only way to use NS_ConvertUTF16toUTF8 is by passing the UTF16 string into
the constructor which does the conversion to UTF8 and then we'll try to do the
conversion to the document character set.
Fair enough :)
Fixed. Please test nightlies and file bugs if anything's wrong.

Mihail: I hope you will write that howto :-). The hash is computed based on the
string encoded with the document's encoding.
We'll probably also need a blurb for the release notes.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago20 years ago
Resolution: --- → FIXED
It works!
(tested with http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/2004-03-07-
09-trunk/mozilla-win32-installer.exe)
I'm so happy!!!
Thanks Peter, Kai and everybody else.
I've tested this fix with Mozilla 1.7 beta, and unfortunately the behavior is 
not exactly backward compatible with NS 4.7. The old NS 4.7 can render HTML 
tags that are part of the "string to sign", when it displays the string for 
confirmation. The new fix for Mozilla however only displays plain text for the 
confirmation of the "string to sign".

At my company, we have developed a general product for integrating with web 
sites to enable form-signing. We make use of this HTML rendering to nicely 
format the "string to sign", for display during the confirmation. This used to 
work with NS 4.7, and we were hoping the current fix would again bring Mozilla 
in the picture.

Since this issue is quite important to us, I'm kindly asking to re-open this 
bug, with the question: can this HTML rendering still be fixed (enhanced)? If 
so, can this still be included in the 1.7?
Johan, please create a new bug for that HTML rendering issue.
Johan, please, do not ask for html rendering.

Html rendering would be very, very, very dangerous because users would
be forced to sign things that they don't see (html tags).

When you sign something, you want to see exactly what you sign. This
can only be achieved with plaintext.

So, please, do not ask for html rendering. If you want to nicely
format the string to sign, use "ascii art".
Ok, I agree that plaintext confirmation text is safer than rendering HTML. It 
would be even safer to display just the raw bytes, which is what is actually 
signed :-), to avoid charset and encoding issues. No seriously, I agree that 
plaintext is anyway superior in security to HTML.

The only issue we had was that we expected backward compatibility with NS 4.7, 
which is now not exactly the case. However, it's no use lamenting over it, we 
will probably change our product's behavior to format its sign text in 
plaintext, rather than in HTML, so we can support Mozilla in the future.
I had the same request as Johan (see comment #16), a new bug was opened for this
issue (I forgot the number) and then closed again with the same argument as José
(dangerous to sign things people don't see...). I'm still not very convinced by
this argument:

First of all other products happily allow signing of html (Microsoft's capicom,
Baltimore and NS 4.7...).

Secondly if you sign something in html with some tags that are not visible (for
example some hidden tags), then to the client it would be simple to show
afterwards in court that these tags were not visible (as they were hidden...). 

I think the risk is similar of signing a paper form. You could argue that paper
is really dangerous as somebody could add some text with invisible ink that
would only show up a day later after you signed. 

Anyway as Johan I'm really happy that Mozilla will have signtext capabilities
now (but I'm not sure that my company is willing to change their HTML form to
plaintext, but I promise to do my best :-). 
I have downloaded 1.7beta and test it with my FNMT (Spanish Oficial
Certification Authority) trying to modify my personal data in the FNMT archives.
It does not work. It is strange since Wamcom 1.3.1 works perfectly.
In fact, 1.7beta does not even reach the actual signtext page. According to
someone else, the data in the formulary are posted through the 3001 port instead
of the expected https port (443) and because of this, there is not a reply from
the server.
Should this bug be reopened or is it a new bug?
It seems that if you use relative URLs for the POST action instead of absolute
ones, it tries to connect to port 3001 instead of 443 (https).
Miguel and Álvaro, the problem that you describe in your coments
is not related to signtext.

Let me insist on this: SIGNTEXT WORKS PERFECTLY NOW. This bug is resolved.
A note to add to the bottom of this, just to confirm the details contained in
the 102 comments above this one of exactly how Mozilla currently behaves. As I
struggled like mad to find out the precise behaviour of signText, here's hoping
this will help someone else.

Both Mozilla/Windows and Mozilla/Linux perform the same way with crypto.signText():

- Strings returned within form variables are terminated with Windows-style CRLF
characters. This happens regardless of whether the original html page was
terminated CRLF, or LF. (If memory serves the HTTP spec says that this must be so).

- The string used when Mozilla signs the text is terminated with whatever the
webserver delivered to it verbatim. In other words, if your original string is
embedded in a unix based html file and terminated LF, it will be signed as is.

This means that when verifying the signed content of a form variable returned
with a request, you need to make sure the end of line characters are converted
to CRLF first.
let not will html-tag rendering, but html-xml-like unicode notation (&#0000) 
gave more compatibility with netscape 4.x
(In reply to comment #103)
> Both Mozilla/Windows and Mozilla/Linux perform the same way with
crypto.signText():
> 
> - Strings returned within form variables are terminated with Windows-style CRLF
> characters. This happens regardless of whether the original html page was
> terminated CRLF, or LF. (If memory serves the HTTP spec says that this must be
so).
> 
> - The string used when Mozilla signs the text is terminated with whatever the
> webserver delivered to it verbatim. In other words, if your original string is
> embedded in a unix based html file and terminated LF, it will be signed as is.
> 
> This means that when verifying the signed content of a form variable returned
> with a request, you need to make sure the end of line characters are converted
> to CRLF first.

Can someone check what 4.x did and file a bug if we're not backwards-compatible
(cc me)?
> Can someone check what 4.x did and file a bug
> if we're not backwards-compatible

Don't worry, Peter.
signtext works perfectly: it signs string as is
(if string lines are terminated LF, signtext don't
add CR, that's all right).

Graham, if you prefer the "LF style",
just strip CRs of the returned form variables
before constructing and verifying the string
used with singtext. On the other hand,
if you prefer "CRLF style", just be sure
to use CRLF everywhere.   :-)

I've made a web aplication that uses signtext
and I prefer to strip CRs of the returned
form variables, so I use LF everywhere
instead of CRLF, because I like the unix
way of life.   :-)

Let me say this again: This bug is resolved.
signtext works perfectly.

Thank you very much, Peter, for your work.
You are the best!    ;-)
Product: PSM → Core
Version: psm2.0 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: