Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(chatform): sanitize the toxstring to remove characters after the \0 #513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/chatlog/chatmessage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ ChatMessage::Ptr ChatMessage::createChatMessage(const QString& sender, const QSt
QString senderText = sender;

auto textType = Text::NORMAL;
// TRIfA suffixes
text = TextFormatter::processTrifaSuffixes(text, settings.getHideTrifaSuffix());

// smileys
if (settings.getUseEmoticons())
text = smileyPack.smileyfied(text);
Expand Down
23 changes: 23 additions & 0 deletions src/chatlog/textformatter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ const QVector<QRegularExpression> URI_WORD_PATTERNS = {
QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(ed2k://\|file\|\S+))")),
};

const QRegularExpression TRIFA_WRAPPER(QStringLiteral("[[]TRIfA_SUFFIX[]](.+)[[]/TRIfA_SUFFIX[]]$"),
QRegularExpression::DotMatchesEverythingOption);
const QString TRIFA_PLACEHOLDER = QStringLiteral("<font color=#228B22>[...]</font>");

struct MatchingUri
{
Expand Down Expand Up @@ -242,3 +245,23 @@ QString TextFormatter::applyMarkdown(const QString& message, bool showFormatting
}
return result;
}

/**
* @brief Remove or show suffixes added by TRIfA
* @param message Formatting string
* @param hideTrifaSuffix True if suffixes should be replaced by [...]
* @return Copy of message where <TRIfA_SUFFIX> tags are removed and and suffix
* is replaced by [...], or remained as is.
*/
QString TextFormatter::processTrifaSuffixes(const QString& message, bool hideTrifaSuffix)
{
QString result = message;
if (hideTrifaSuffix) {
return result.replace(TRIFA_WRAPPER, TRIFA_PLACEHOLDER);
}
QRegularExpressionMatch match = TRIFA_WRAPPER.match(result);
if (match.hasMatch()) {
result = result.replace(TRIFA_WRAPPER, match.captured(match.lastCapturedIndex()));
}
return result;
}
2 changes: 2 additions & 0 deletions src/chatlog/textformatter.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,6 @@ namespace TextFormatter {
QString highlightURI(const QString& message);

QString applyMarkdown(const QString& message, bool showFormattingSymbols);

QString processTrifaSuffixes(const QString& message, bool hideTrifaSuffix);
} // namespace TextFormatter
4 changes: 2 additions & 2 deletions src/core/core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ void Core::onFriendMessage(Tox* tox, uint32_t friendId, Tox_Message_Type type,
{
std::ignore = tox;
const bool isAction = (type == TOX_MESSAGE_TYPE_ACTION);
const QString msg = ToxString(cMessage, cMessageSize).getQString();
const QString msg = ToxString(cMessage, cMessageSize, true).getQString();
emit static_cast<Core*>(core)->friendMessageReceived(friendId, msg, isAction);
}

Expand Down Expand Up @@ -492,7 +492,7 @@ void Core::onConferenceMessage(Tox* tox, uint32_t conferenceId, uint32_t peerId,
std::ignore = tox;
Core* core = static_cast<Core*>(vCore);
const bool isAction = type == TOX_MESSAGE_TYPE_ACTION;
const QString message = ToxString(cMessage, length).getQString();
const QString message = ToxString(cMessage, length, true).getQString();
emit core->conferenceMessageReceived(conferenceId, peerId, message, isAction);
}

Expand Down
74 changes: 71 additions & 3 deletions src/core/toxstring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include <cassert>
#include <climits>
#include <ctime>
#include <utility>

/**
Expand Down Expand Up @@ -43,9 +44,61 @@ ToxString::ToxString(QByteArray text)
* @param length Number of bytes to read from the beginning.
*/
ToxString::ToxString(const uint8_t* text, size_t length)
: ToxString(text, length, false)
{
assert(length <= INT_MAX);
string = QByteArray(reinterpret_cast<const char*>(text), length);
}


/**
* @brief Creates a ToxString from the representation used by c-toxcore.
* @param text Pointer to the beginning of the text.
* @param length Number of bytes to read from the beginning.
* @param fix_trifa if true, fix TRIfA messages v3, containing suffix after \0.
*/
ToxString::ToxString(const uint8_t* text, size_t length, bool fix_trifa)
{
if (length > SIZE_MAX) {
qCritical() << "The string has invalid length!";
return;
}
if (fix_trifa) {
// Try to detect TRIfA message v3 format:
// message itself
// \0\0 TOX_MSGV3_GUARD = 2
// random bytes TOX_MSGV3_MSGID_LENGTH = 32
// time stamp TOX_MSGV3_TIMESTAMP_LENGTH = 4
size_t actualLength = 0;
for (actualLength = 0; actualLength < length; actualLength++) {
if (*(text + actualLength) == 0)
break;
}
// Only trim string if its suffix looks like a TRIfA signature.
if (length - actualLength == 2 + 32 + 4 && *(text + actualLength + 1) == 0) {
// Convert the uint8[4] to timestamp to string
std::time_t trifa_timestamp = 0;
for (int i = 4; i > 0; i--) {
trifa_timestamp = trifa_timestamp << 8;
trifa_timestamp += *(text + length - i);
}
string = QByteArray(reinterpret_cast<const char*>(text), actualLength);
// Mark TRIFA suffix to remove it or replace by format symbol
string.append("[TRIfA_SUFFIX]");
// Skip two \0 symbols.
// The string contains random characters and we need to sanitize it.
QByteArray randomBt(reinterpret_cast<const char*>(text + actualLength + 2), 32);
string.append(getQString(randomBt).toLocal8Bit().constData());
// Convert time to string
char strTime[std::size("yyyy-mm-ddThh:mm:ssZ")];
std::strftime(std::data(strTime), std::size(strTime), "%Y-%m-%dT%H:%M:%SZ",
std::gmtime(&trifa_timestamp));
string.append(strTime);
string.append("[/TRIfA_SUFFIX]");
} else {
string = QByteArray(reinterpret_cast<const char*>(text), length);
}
} else {
string = QByteArray(reinterpret_cast<const char*>(text), length);
}
}

/**
Expand All @@ -71,10 +124,25 @@ size_t ToxString::size() const
*
* Removes any non-printable characters from the string. This is a defense-in-depth measure to
* prevent some potential security issues caused by bugs in client code or one of its dependencies.
* @return the string with non printable symbols removed.
*/
QString ToxString::getQString() const
{
const auto tainted = QString::fromUtf8(string).toStdU32String();
return getQString(string);
}


/**
* @brief Interpret the string as UTF-8 encoded QString.
*
* Removes any non-printable characters from the string. This is a defense-in-depth measure to
* prevent some potential security issues caused by bugs in client code or one of its dependencies.
* @param stringVal the buffer with bytes.
* @return the string with non printable symbols removed.
*/
QString ToxString::getQString(QByteArray stringVal) const
{
const auto tainted = QString::fromUtf8(stringVal).toStdU32String();
QSet<std::pair<QChar::Category, char32_t>> removed;
std::u32string cleaned;
std::copy_if(tainted.cbegin(), tainted.cend(), std::back_inserter(cleaned), [&removed](char32_t c) {
Expand Down
2 changes: 2 additions & 0 deletions src/core/toxstring.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ class ToxString
explicit ToxString(const QString& text);
explicit ToxString(QByteArray text);
ToxString(const uint8_t* text, size_t length);
ToxString(const uint8_t* text, size_t length, bool fix_trifa);

const uint8_t* data() const;
size_t size() const;
QString getQString() const;
QString getQString(QByteArray stringVal) const;
const QByteArray& getBytes() const;

private:
Expand Down
7 changes: 6 additions & 1 deletion src/model/notificationgenerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

namespace {

const QRegularExpression TRIFA_WRAPPER(QStringLiteral("[[]TRIfA_SUFFIX[]](.+)[[]/TRIfA_SUFFIX[]]$"),
QRegularExpression::DotMatchesEverythingOption);

QString generateContent(const QHash<const Conference*, size_t>& conferenceNotifications,
QString lastMessage, const ToxPk& sender)
{
Expand Down Expand Up @@ -51,7 +54,9 @@ NotificationData NotificationGenerator::friendMessageNotification(const Friend*
}

ret.title = f->getDisplayedName();
ret.message = message;
// Make sure we have removed the TRIfA suffix if any.

ret.message = QString(message).remove(TRIFA_WRAPPER);
ret.pixmap = getSenderAvatar(profile, f->getPublicKey());

return ret;
Expand Down
15 changes: 15 additions & 0 deletions src/persistence/settings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ void Settings::loadGlobal()
imagePreview = s.value("imagePreview", true).toBool();
chatMaxWindowSize = s.value("chatMaxWindowSize", 100).toInt();
chatWindowChunkSize = s.value("chatWindowChunkSize", 50).toInt();
hideTrifaSuffix = s.value("hideTrifaSuffix", true).toBool();
});

inGroup(s, "Chat", [this, &s] {
Expand Down Expand Up @@ -690,6 +691,7 @@ void Settings::saveGlobal()
s.setValue("statusChangeNotificationEnabled", statusChangeNotificationEnabled);
s.setValue("showConferenceJoinLeaveMessages", showConferenceJoinLeaveMessages);
s.setValue("spellCheckingEnabled", spellCheckingEnabled);
s.setValue("hideTrifaSuffix", hideTrifaSuffix);
});

inGroup(s, "Chat", [this, &s] { //
Expand Down Expand Up @@ -1443,6 +1445,19 @@ void Settings::setChatMessageFont(const QFont& font)
}
}

bool Settings::getHideTrifaSuffix() const
{
const QMutexLocker<QRecursiveMutex> locker(&bigLock);
return hideTrifaSuffix;
}

void Settings::setHideTrifaSuffix(bool hide)
{
if (setVal(hideTrifaSuffix, hide)) {
emit hideTrifaSuffixChanged(hide);
}
}

void Settings::setWidgetData(const QString& uniqueName, const QByteArray& data)
{
bool updated = false;
Expand Down
5 changes: 5 additions & 0 deletions src/persistence/settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ public slots:
void dateFormatChanged(const QString& format);
void statusChangeNotificationEnabledChanged(bool enabled);
void spellCheckingEnabledChanged(bool enabled);
void hideTrifaSuffixChanged(bool show);

// Privacy
void typingNotificationChanged(bool enabled);
Expand Down Expand Up @@ -458,6 +459,9 @@ public slots:
bool getSpellCheckingEnabled() const;
void setSpellCheckingEnabled(bool newValue);

bool getHideTrifaSuffix() const;
void setHideTrifaSuffix(bool hide);

// Privacy
bool getTypingNotification() const;
void setTypingNotification(bool enabled);
Expand Down Expand Up @@ -671,6 +675,7 @@ private slots:
bool statusChangeNotificationEnabled;
bool showConferenceJoinLeaveMessages;
bool spellCheckingEnabled;
bool hideTrifaSuffix;

// Privacy
bool typingNotification;
Expand Down
6 changes: 6 additions & 0 deletions src/widget/form/settings/userinterfaceform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ UserInterfaceForm::UserInterfaceForm(SmileyPack& smileyPack_, Settings& settings
bodyUI->chatLogMaxTxt->setValue(settings.getChatMaxWindowSize());
bodyUI->chatLogChunkTxt->setValue(settings.getChatWindowChunkSize());
bodyUI->cbImagePreview->setChecked(settings.getImagePreview());
bodyUI->cbHideTrifaSuffix->setChecked(settings.getHideTrifaSuffix());

bodyUI->cbConferencePosition->setChecked(settings.getConferencePosition());
bodyUI->cbCompactLayout->setChecked(settings.getCompactLayout());
Expand Down Expand Up @@ -399,6 +400,11 @@ void UserInterfaceForm::on_cbImagePreview_stateChanged()
settings.setImagePreview(bodyUI->cbImagePreview->isChecked());
}

void UserInterfaceForm::on_cbHideTrifaSuffix_stateChanged()
{
settings.setHideTrifaSuffix(bodyUI->cbHideTrifaSuffix->isChecked());
}

void UserInterfaceForm::on_chatLogChunkTxt_valueChanged(int value)
{
settings.setChatWindowChunkSize(value);
Expand Down
1 change: 1 addition & 0 deletions src/widget/form/settings/userinterfaceform.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ private slots:
void on_txtChatFontSize_valueChanged(int px);
void on_useNameColors_stateChanged(int value);
void on_cbImagePreview_stateChanged();
void on_cbHideTrifaSuffix_stateChanged();
void on_chatLogMaxTxt_valueChanged(int value);
void on_chatLogChunkTxt_valueChanged(int value);

Expand Down
19 changes: 13 additions & 6 deletions src/widget/form/settings/userinterfacesettings.ui
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>664</width>
<height>993</height>
<width>650</width>
<height>1104</height>
</rect>
</property>
<layout class="QVBoxLayout" name="verticalLayout_4" stretch="0,0,0,0,0,0">
Expand Down Expand Up @@ -167,6 +167,13 @@ Hide formatting characters:
</property>
</widget>
</item>
<item row="3" column="0">
<widget class="QLabel" name="label">
<property name="text">
<string>Chat log:</string>
</property>
</widget>
</item>
<item row="3" column="1">
<widget class="QCheckBox" name="cbImagePreview">
<property name="toolTip">
Expand All @@ -177,7 +184,7 @@ Hide formatting characters:
</property>
</widget>
</item>
<item row="4" column="1">
<item row="5" column="1">
<layout class="QHBoxLayout" name="horizontalLayout_4">
<item>
<widget class="QLabel" name="chatLogMaxLbl">
Expand Down Expand Up @@ -224,10 +231,10 @@ number here may cause the scroll bar to disappear.</string>
</item>
</layout>
</item>
<item row="3" column="0">
<widget class="QLabel" name="label">
<item row="4" column="1">
<widget class="QCheckBox" name="cbHideTrifaSuffix">
<property name="text">
<string>Chat log:</string>
<string>Hide TRIfA suffix</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually TRIfA-specific? Can we call it something more generic, like post-null suffix or something?

</property>
</widget>
</item>
Expand Down
Loading
Loading