locka99 / opcua

A client and server implementation of the OPC UA specification written in Rust
Mozilla Public License 2.0
496 stars 131 forks source link

OPC UA client fails to store server certificate if common name (CN) contains forward slash #133

Closed jonasar closed 3 years ago

jonasar commented 3 years ago

This was tested on opcua_client v 0.8

In function cert_file_name() in crypto/src/certificate_store.rs, the file name is concatenated from the certificate common name and the thumbprint, with spaces between them (!)·

Spaces in filenames are not valid on Linux.

The bug is on this line: https://github.com/locka99/opcua/blob/1dbca0e24759355a77c1388f3f6d2883cd745127/crypto/src/certificate_store.rs#L424

Here is the location of the same bug in version 0.8: https://docs.rs/opcua-crypto/0.8.0/src/opcua_crypto/certificate_store.rs.html#328

Also if the common name includes invalid characters for filenames, e.g. forward slash "/", the certificate will not be stored correctly.

jonasar commented 3 years ago

I can create a pull-request with a fix if you give me necessary permission on the repo.

I suggest replacing the spaces with dots and also skipping the square brackets "[" "]" in the file name.

locka99 commented 3 years ago

Most Linux filesystems supports pretty much every character with perhaps the exception of forward slashes or special names like "." and "..". So I'd accept a patch that strips out the "/" but other chars including whitespaces should remain the way they are. Whitespaces in Linux can be used although from a command prompt you might have to escape them, e.g. " " becomes "\ " as you type it.

locka99 commented 3 years ago

A further note - I don't know what the encoding rules are for X509 regarding unicode chars in the CN but Linux typically allows UTF-8 encoded filenames so providing the field comes out as valid unicode we should also assume that to be a valid filename also aside from the forward slash.

jonasar commented 3 years ago

You are right, whitespace is allowed in filenames and it was actually only the forward slash in the CN that caused the opcua client to fail when trying to store the server certificate.

(I would still argue that it is not good practice to use spaces and square brackets in file names, but perhaps that is a matter of taste)

jonasar commented 3 years ago

Here is the patch:

diff --git a/crypto/src/certificate_store.rs b/crypto/src/certificate_store.rs
index ad757f62..8bd3cb2b 100644
--- a/crypto/src/certificate_store.rs
+++ b/crypto/src/certificate_store.rs
@@ -414,7 +414,7 @@ impl CertificateStore {
     /// the cert's common name being empty or not
     pub fn cert_file_name(cert: &X509) -> String {
         let prefix = if let Ok(common_name) = cert.common_name() {
-            common_name.trim().to_string()
+            common_name.trim().to_string().replace("/", "")
         } else {
             String::new()
         };

I would be very grateful if you would be willing to release a patch version based on 0.8 on crates.io including this fix soon.

locka99 commented 3 years ago

I'll publish 0.8.1 for this and it has landed on 0.9