Sanitize peer client names

[HanabishiRecca]

Clients can send arbitrary UTF-8 strings as their client identificators. qBittorrent displays that strings as is, which could lead to unexpected results and affect GUI layout in case of junk or intentionally malicious strings.

This PR introduces basic sanitization for client strings.

• Source string is simplified.
• Rest non-printable characters are removed.

Fixes #20010

[glassez]

qBittorrent displays that strings as is, which could lead to unexpected results and affect GUI layout in case of junk or intentionally malicious strings.

If the problem is displaying of such a data then it should be sanitized at the layer where this (displaying) is done.

[HanabishiRecca]

If the problem is displaying of such a data then it should be sanitized at the layer where this (displaying) is done.

Are you sure? There is no way to reliably do that in JS for WebUI. For regualr GUI it probably will require extra overhead with string copies. Consider that the value is never used in code, only for displaying in UI, so keeping original value is pointless.

We already do sanitization in-place e.g. for peer id. Libtorrent also does similar things on its side.

[glassez]

Consider that the value is never used in code, only for displaying in UI, so keeping original value is pointless.

I disagree with you in general. But considering that in this particular case, this field is unlikely to be intended for anything other than a visual representation of the client name, I agree to leave it as it is. (If we really need something more unique, then we should use the raw peer ID.)

[glassez]

It uses QChar::isPrint() to replace all non-printable characters with spaces. I'm not sure if it accounts for all possible dangerous Unicode sequences, feel free to advice improvements.

Fixes #20010

Can you confirm that it actually fixes #20010?

[Chocobo1]

qBittorrent displays that strings as is, which could lead to unexpected results and affect GUI layout in case of junk or intentionally malicious strings.

https://www.libtorrent.org/reference-Core.html#peer_info states:

client
A human readable string describing the software at the other end of the connection. In some cases this information is not available, then it will contain a string that may give away something about which software is running in the other end. In the case of a web seed, the server type and version will be a part of this string. This is UTF-8 encoded.

IMO libtorrent do try to make it human readable and if that is the case then shouldn't this PR applied at libtorrent side?

[HanabishiRecca]

this field is unlikely to be intended for anything other than a visual representation of the client name

Exactly. It's an arbitrary string sent by a remote client. We should never ever rely or make assumptions about it.

Can you confirm that it actually fixes #20010?

Yes, I tested it artifically. It sanitizes \n characters.

shouldn't this PR applied at libtorrent side?

Can't say. Some other clients using libtorrent may want to see the original string. Who knows.

[Chocobo1]

qBittorrent/src/gui/properties/peerlistwidget.cpp

Line 483 in 0541645

const QString client = peer.client().toHtmlEscaped();

The GUI side has escaping for HTML entities. If sanitizing are to be done at PeerInfo::client() then you should move it (toHtmlEscaped()) over. IIRC WebUI has escaping too so it would need to be removed to avoid double escaping.

[HanabishiRecca]

If sanitizing are to be done at PeerInfo::client() then you should move it (toHtmlEscaped()) over.

It will be kinda inconsistient with the rest of the code. https://github.com/search?q=repo%3Aqbittorrent%2FqBittorrent%20toHtmlEscaped&type=code

We probably should do it for all fields then in a separate PR?

[Chocobo1]

If sanitizing are to be done at PeerInfo::client() then you should move it (toHtmlEscaped()) over.

It will be kinda inconsistient with the rest of the code. https://github.com/search?q=repo%3Aqbittorrent%2FqBittorrent%20toHtmlEscaped&type=code

We are only talking about the client field in this PR. It comes back to this #20788 (comment)

We probably should do it for all fields then in a separate PR?

I would prefer changes to client field is finished in this PR. Can't say my opinion for other fields now.

[HanabishiRecca]

Ok. Changes:

• Source string is simplified().
• Space-like characters (\t, \n, \v, \f, \r ...) are replaced with regular whitespaces.
• Rest non-printable characters are omitted completely.
• toHtmlEscaped() interned into the method.

[glassez]

The GUI side has escaping for HTML entities. If sanitizing are to be done at PeerInfo::client() then you should move it (toHtmlEscaped()) over.

But this certainly shouldn't be in the base layer. The reasoning that it is not (yet) used without toHtmlEscaped() is invalid in terms of the correct application architecture. The need for HTML escaping is an attribute of some display methods, not the model. Although in this case the purpose of this aspect of the model is to provide a readable value, HTML escaping is not part of this generally.

[HanabishiRecca]

Although in this case the purpose of this aspect of the model is to provide a readable value, HTML escaping is not part of this generally.

I tend to agree with that.

[glassez]

@HanabishiRecca
Thank you!