Skip to content

OF-2163: Prevent unexpected stanza modifications to leak information #1770

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 17, 2020
Merged

OF-2163: Prevent unexpected stanza modifications to leak information #1770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions xmppserver/src/main/java/org/jivesoftware/openfire/muc/HistoryRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ public void sendHistory(MUCRole joinRole, MUCRoomHistory roomHistory) {
if (!isConfigured()) {
Iterator<Message> history = roomHistory.getMessageHistory();
while (history.hasNext()) {
joinRole.send(history.next());
// OF-2163: Create a defensive copy of the message, to prevent the address that it is sent to to leak back into the archive.
joinRole.send(history.next().createCopy());
}
}
else {
Expand Down Expand Up @@ -194,8 +195,9 @@ public void sendHistory(MUCRole joinRole, MUCRoomHistory roomHistory) {
historyToSend.addFirst(message);
}
// Send the smallest amount of traffic to the user
for (Object aHistoryToSend : historyToSend) {
joinRole.send((Message) aHistoryToSend);
for (final Message aHistoryToSend : historyToSend) {
// OF-2163: Create a defensive copy of the message, to prevent the address that it is sent to to leak back into the archive.
joinRole.send(aHistoryToSend.createCopy());
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,12 @@ default boolean isRemoteFmuc() {
/**
* Sends a packet to the user.
*
* Note that sending a packet can modify it (notably, the 'to' address can be changed. If this is undesired (for
* example, because post-processing should not expose the modified 'to' address), then a copy of the original
* stanza should be provided as an argument to this method.
*
* @param packet The packet to send
* @see <a href="https://igniterealtime.atlassian.net/browse/OF-2163">issue OF-2163</a>
*/
void send( Packet packet );

Expand Down
2 changes: 2 additions & 0 deletions xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/IQMUCvCardHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ private void sendVCardUpdateNotification( final MUCRoom room, String hash )

for ( final MUCRole occupant : room.getOccupants() )
{
// Not needed to create a defensive copy of the stanza. It's not used anywhere else.
occupant.send(notification);
}
}
Expand All @@ -245,6 +246,7 @@ private void sendConfigChangeNotification( final MUCRoom room )

for ( final MUCRole occupant : room.getOccupants() )
{
// Not needed to create a defensive copy of the stanza. It's not used anywhere else.
occupant.send(notification);
}
}
Expand Down
88 changes: 56 additions & 32 deletions xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/LocalMUCRoom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,8 @@ public MUCRole joinRoom( @Nonnull String nickname,
final Presence presenceItemNotFound = new Presence(Presence.Type.error);
presenceItemNotFound.setError(PacketError.Condition.item_not_found);
presenceItemNotFound.setFrom(role.getRoleAddress());

// Not needed to create a defensive copy of the stanza. It's not used anywhere else.
joinRole.send(presenceItemNotFound);
}

Expand All @@ -722,7 +724,10 @@ private void sendRoomHistoryAfterJoin( @Nonnull final LocalMUCUser user, @Nonnul
Log.trace( "Sending default room history to user '{}' that joined room '{}'.", user.getAddress(), this.getJID() );
final Iterator<Message> history = roomHistory.getMessageHistory();
while (history.hasNext()) {
joinRole.send(history.next());
// OF-2163: Prevent modifying the original history stanza (that can be retrieved by others later) by making a defensive copy.
// This prevents the stanzas in the room history to have a 'to' address for the last user that it was sent to.
final Message message = history.next().createCopy();
joinRole.send(message);
}
} else {
Log.trace( "Sending user-requested room history to user '{}' that joined room '{}'.", user.getAddress(), this.getJID() );
Expand All @@ -736,7 +741,10 @@ private void sendRoomHistoryAfterJoin( @Nonnull final LocalMUCUser user, @Nonnul
private void sendRoomSubjectAfterJoin( @Nonnull final LocalMUCUser user, @Nonnull MUCRole joinRole )
{
Log.trace( "Sending room subject to user '{}' that joined room '{}'.", user.getAddress(), this.getJID() );
Message roomSubject = roomHistory.getChangedSubject();

// OF-2163: Prevent modifying the original subject stanza (that can be retrieved by others later) by making a defensive copy.
// This prevents the stanza kept in memory to have the 'to' address for the last user that it was sent to.
Message roomSubject = roomHistory.getChangedSubject().createCopy();

// 7.2.15 If there is no subject set, the room MUST return an empty <subject/> element.
if (roomSubject == null) {
Expand Down Expand Up @@ -1034,14 +1042,11 @@ void sendInitialPresencesToNewOccupant(MUCRole joinRole) {
}
}

final Presence occupantPresence;
final Presence occupantPresence = occupant.getPresence().createCopy(); // defensive copy to prevent modifying the original.
if (!canAnyoneDiscoverJID() && MUCRole.Role.moderator != joinRole.getRole()) {
// Don't include the occupant's JID if the room is semi-anon and the new occupant is not a moderator
occupantPresence = occupant.getPresence().createCopy(); // defensive copy to prevent modifying the original.
final Element frag = occupantPresence.getChildElement("x", "http://jabber.org/protocol/muc#user");
frag.element("item").addAttribute("jid", null);
} else {
occupantPresence = occupant.getPresence();
}
joinRole.send(occupantPresence);
}
Expand Down Expand Up @@ -1086,8 +1091,8 @@ CompletableFuture<Void> sendLeavePresenceToExistingOccupants(MUCRole leaveRole)
// Send the presence of this new occupant to existing occupants
Log.trace( "Send presence of leaving occupant '{}' to existing occupants of room '{}'.", leaveRole.getUserAddress(), leaveRole.getChatRoom().getJID() );
try {
Presence originalPresence = leaveRole.getPresence();
Presence presence = originalPresence.createCopy();
final Presence originalPresence = leaveRole.getPresence();
final Presence presence = originalPresence.createCopy(); // Defensive copy to not modify the original.
presence.setType(Presence.Type.unavailable);
presence.setStatus(null);
// Change (or add) presence information about roles and affiliations
Expand Down Expand Up @@ -1326,13 +1331,12 @@ public void destroyRoom(DestroyRoomRequest destroyRequest) {
for (MUCRole removedRole : removedRoles) {
try {
// Send a presence stanza of type "unavailable" to the occupant
Presence presence = createPresence(Presence.Type.unavailable);
final Presence presence = createPresence(Presence.Type.unavailable);
presence.setFrom(removedRole.getRoleAddress());

// A fragment containing the x-extension for room destruction.
Element fragment = presence.addChildElement("x",
"http://jabber.org/protocol/muc#user");
Element item = fragment.addElement("item");
final Element fragment = presence.addChildElement("x","http://jabber.org/protocol/muc#user");
final Element item = fragment.addElement("item");
item.addAttribute("affiliation", "none");
item.addAttribute("role", "none");
if (alternateJID != null) {
Expand All @@ -1345,6 +1349,8 @@ public void destroyRoom(DestroyRoomRequest destroyRequest) {
}
destroy.addElement("reason").setText(reason);
}

// Not needed to create a defensive copy of the stanza. It's not used anywhere else.
removedRole.send(presence);
}
catch (Exception e) {
Expand Down Expand Up @@ -1418,16 +1424,24 @@ public void sendPrivatePacket(Packet packet, MUCRole senderRole) throws NotFound
if (occupants == null || occupants.size() == 0) {
throw new NotFoundException();
}
if (canAnyoneDiscoverJID && packet instanceof Message) {
addRealJidToMessage((Message)packet, senderRole);

// OF-2163: Prevent modifying the original stanza (that can be used by unrelated code, after this method returns) by making a defensive copy.
final Packet stanza = packet.createCopy();
if (canAnyoneDiscoverJID && stanza instanceof Message) {
addRealJidToMessage((Message)stanza, senderRole);
}
for (MUCRole occupant : occupants) {
packet.setFrom(senderRole.getRoleAddress());
occupant.send(packet);
if(packet instanceof Message) {
Message message = (Message) packet;
MUCEventDispatcher.privateMessageRecieved(occupant.getUserAddress(), senderRole.getUserAddress(),
message);
stanza.setFrom(senderRole.getRoleAddress());

// Sending the stanza will modify it. Make sure that the event listeners that are triggered after sending
// the stanza don't get the 'real' address from the recipient.
final Packet immutable = stanza.createCopy();

// Forward it to each occupant.
for (final MUCRole occupant : occupants) {
occupant.send(stanza); // Use the stanza copy to send data. The 'to' address of this object will be changed by sending it.
if (stanza instanceof Message) {
// Use an unmodified copy of the stanza (with the original 'to' address) when invoking event listeners (OF-2163)
MUCEventDispatcher.privateMessageRecieved(occupant.getUserAddress(), senderRole.getUserAddress(), (Message) immutable);
}
}
}
Expand Down Expand Up @@ -1491,28 +1505,31 @@ private CompletableFuture<Void> broadcastPresence( Presence presence, boolean is
throw new IllegalArgumentException("Broadcasted presence stanza's 'from' JID " + presence.getFrom() + " does not match room JID: " + this.getJID());
}

// Create a defensive copy, to prevent modifications to leak back to the invoker.
final Presence stanza = presence.createCopy();

// Some clients send a presence update to the room, rather than to their own nickname.
if ( JiveGlobals.getBooleanProperty("xmpp.muc.presence.overwrite-to-room", true) && presence.getTo() != null && presence.getTo().getResource() == null && sender.getRoleAddress() != null) {
presence.setTo( sender.getRoleAddress() );
if ( JiveGlobals.getBooleanProperty("xmpp.muc.presence.overwrite-to-room", true) && stanza.getTo() != null && stanza.getTo().getResource() == null && sender.getRoleAddress() != null) {
stanza.setTo( sender.getRoleAddress() );
}

if (!shouldBroadcastPresence(presence)) {
if (!shouldBroadcastPresence(stanza)) {
// Just send the presence to the sender of the presence
sender.send(presence);
sender.send(stanza);
return CompletableFuture.completedFuture(null);
}

// If FMUC is active, propagate the presence through FMUC first. Note that when a master-slave mode is active,
// we need to wait for an echo back, before the message can be broadcasted locally. The 'propagate' method will
// return a CompletableFuture object that is completed as soon as processing can continue.
return fmucHandler.propagate(presence, sender)
return fmucHandler.propagate(stanza, sender)
.thenRunAsync(() -> {
// Broadcast presence to occupants hosted by other cluster nodes
BroadcastPresenceRequest request = new BroadcastPresenceRequest(this, sender, presence, isJoinPresence);
BroadcastPresenceRequest request = new BroadcastPresenceRequest(this, sender, stanza, isJoinPresence);
CacheFactory.doClusterTask(request);

// Broadcast presence to occupants connected to this JVM
request = new BroadcastPresenceRequest(this, sender, presence, isJoinPresence);
request = new BroadcastPresenceRequest(this, sender, stanza, isJoinPresence);
request.setOriginator(true);
request.run();
}
Expand Down Expand Up @@ -1687,14 +1704,18 @@ public void broadcast(@Nonnull final BroadcastMessageRequest messageRequest)
// Add message to the room history
roomHistory.addMessage(message);
// Send message to occupants connected to this JVM
for (MUCRole occupant : occupantsByFullJID.values()) {

// Create a defensive copy of the message that will be broadcast, as the broadcast will modify it ('to' addresses
// will be changed), and it's undesirable to see these modifications in post-processing (OF-2163).
final Message mutatingCopy = message.createCopy();
for (final MUCRole occupant : occupantsByFullJID.values()) {
try
{
// Do not send broadcast messages to deaf occupants or occupants hosted in
// other cluster nodes or other FMUC nodes.
if ( occupant.isLocal() && !occupant.isVoiceOnly() && !occupant.isRemoteFmuc() )
{
occupant.send( message );
occupant.send( mutatingCopy );
}
}
catch ( Exception e )
Expand Down Expand Up @@ -2837,8 +2858,11 @@ private void kickPresence(Presence kickPresence, JID actorJID, String nick) {
actor.addAttribute("nick", nick);
}
}
// Send the unavailable presence to the banned user
kickedRole.send(kickPresence);

// Send a defensive copy (to not leak a change to the 'to' address - this is possibly overprotective here,
// but we're erring on the side of caution) of the unavailable presence to the banned user.
kickedRole.send(kickPresence.createCopy());

// Remove the occupant from the room's occupants lists
OccupantLeftEvent event = new OccupantLeftEvent(this, kickedRole);
event.setOriginator(true);
Expand Down
3 changes: 2 additions & 1 deletion xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,8 @@ private void broadcastShutdown()
item.addAttribute( "role", "none" );
fragment.addElement( "status" ).addAttribute( "code", "332" );

// Make sure that the presence change for each user is only sent to that user (and not broadcasted in the room)!
// Make sure that the presence change for each user is only sent to that user (and not broadcast in the room)!
// Not needed to create a defensive copy of the stanza. It's not used anywhere else.
role.send( presence );
}
}
Expand Down