Subject: Re: [xmpp] Robert Sparks' Discuss on draft-ietf-xmpp-3921bis-19: (with DISCUSS and COMMENT)

From: Peter Saint-Andre <[email-masked]>
Date: Thu, 20 Jan 2011 11:46:47 -0700

[ cc'ing [email-masked] to keep the WG in the loop ]

On 1/19/11 2:02 PM, Robert Sparks wrote:
> Robert Sparks has entered the following ballot position for 
> draft-ietf-xmpp-3921bis-19: Discuss
> 
> When responding, please keep the subject line intact and reply to
> all email addresses included in the To and CC lines. (Feel free to
> cut this introductory paragraph, however.)
> 
> Please refer to
> http://www.ietf.org/iesg/statement/discuss-criteria.html for more
> information about IESG DISCUSS and COMMENT positions.
> 
> 
> 
> ----------------------------------------------------------------------
>
> 
DISCUSS:
> ----------------------------------------------------------------------
>
>  DISCUSS In section 3.1.3 item 4, a server is required to keep a
> complete presence stanza for an unbounded amount of time (potentially
> forever).  This looks like a vector for a state-exhaustion attack.
> The restriction for keeping only one such stanza per sender helps,
> but a malicious attacking server could send stanzas from an arbitrary
> number of identites. > I suggest providing implementation guidance on
> when it's ok to drop information that's truly stale, and call out the
> potential for this attack in the security considerations.

You're right that we had not considered this attack. Possibly also the
attack is not limited to a rogue server -- e.g., a botnet spread across
multiple accounts on one or more servers could flood a victim server or
victim account with presence subscription requests.

I propose the following text:

      Security Warning: The mandate for the contact's server to store
      the complete stanza of the presence subscription request
      introduces the possibility of an application resource exhaustion
      attack (see Section 2.1.2 of [DOS]), for example by a rogue server
      or a coordinated group of users (e.g., a botnet) against the
      contact's server or particular contact.  Server implementors are
      advised to consider the possibility of such attacks and provide
      tools for counteracting it, such as enabling service
      administrators to set limits on the number or size of inbound
      presence subscription requests that the server will store in
      aggregate or for any given contact.

> Section 5.2.4 calls out "for the purpose of providing alternate
> versions of the same subject" when allowing multiple subjects to
> appear with different xml:lang attributes. Section 5.2.3 does not
> have analogous text constraining multiple bodies. Should it?

Yes, it should. Fixed in my working copy.

> Section 5.3 - Was the intent to constrain the contents of a message
> stanza to things intended to be rendered to a user? 

No.

> As written, this
> section would allow <message/> to be used as part of the framing of
> an arbitrary transport protocol.

Yep. Here is an example:

http://xmpp.org/extensions/xep-0047.html

And that works quite well, too! However, we've been working to get folks
to use <iq/> stanzas for in-band bytestreams for better flow control.

> Section 5.3 - The document should say what an implementation
> receiving a child element in a namespace it doesn't understand should
> do with that child.

That's covered in Section 8.4 of draft-ietf-xmpp-3920bis, and inherited
thereby.

I propose modifying the paragraph as follows:

   As described in [XMPP-CORE], an XML stanza MAY contain any child
   element that is qualified by a namespace other than the default
   namespace; this applies to the message stanza as well.  Guidelines
   for handling extended content on the part of both routing servers and
   end recipients are provided in Section 8.4 of [XMPP-CORE].

> ----------------------------------------------------------------------
>
> 
COMMENT:
> ----------------------------------------------------------------------
>
>  COMMENT
> 
> In section 2.3.3, is the intent that error conditions listed earlier
> in the section take precedence over error conditions listed later in
> the section? What's the appropriate response if a message qualifies
> for both a <forbidden/> and a <bad-request/> as described in this
> section?

That up to the implementation. In general, implementations seem to take
security-related transgressions (e.g., <forbidden/>) more seriously than
protocol-related peccadilloes (e.g., <bad-request/>).

> When would an element choose not to follow the SHOULD in 3.1.2 item
> 1, and what are the consequences?

There are two points covered in this paragraph:

1. Whether the JID is a bare JID or a full JID. In some deployments it
might be fine to allow full JIDs to subscribe, however we tend to
discourage that, thus the SHOULD. The consequences mostly relate to the
subscriber, not the subscribee, so as far as I know there is no strong
agreement in the developer community about making this a MUST.

2. Whether the subscriber JID adheres to the JID format. In general, it
is the responsibility of the sending server to authenticate entities
correctly and enforce the JID format, not the responsibility of a server
or end recipient. This is related somewhat to the address spec (and
rfc3920[bis]), which have never been completely clear about which
entities are responsible for enforcing the JID format. However, if
anything that would be the responsibility of the sending server, and
this will be clarified in revisions to the address spec. Probably that
will be a topic at the interim meeting a few weeks from now.

Given my reading of the community, SHOULD is as strong as we can get
here for now.

> The end of section 4.3 has an implementation note containing
> normative requirements. Can these be moved into the main prose? >

Done in my working copy.

> This
> same implementation note may be easy to misinterpret. I don't believe
> it is meant to prevent an implementation from returning an error to a
> malformed presence probe, but it could be read that way.

I propose the following modified paragraph:

   Presence probes SHOULD NOT be sent by a client, because in general a
   client will not need to send them since the task of gathering
   presence from a user's contacts is managed by the user's server.
   However, if a user's client generates an outbound presence probe then
   the user's server SHOULD route the probe (if the contact is at
   another server) or process the probe (if the contact is at the same
   server) and MUST NOT use its receipt of the presence probe from a
   connected client as a cause for returning a stanza or stream error to
   the client.

The last clause speaks to a bug that existed in at least one XMPP
server: if a connected client sent a presence probe then the server
would return a presence error or even terminate the client's session.
Hopefully the revised text is a bit clearer.

> Section 5.2.5 does not prevent a thread from claiming it is its own
> parent (should that be allowed?), or warn implementations about the
> possibility of thread A indicating a parent of B, and thread B
> indicating a parent of A (generalize this to any kind of cycle). I
> don't think XEP-0201 calls this out either. Should it be mentioned as
> an implementation consideration? (A naive implementation would very
> likely crash or do something surprising with its UI.)

You're right that we had not considered that scenario.

Does the following text address your concern?

###

   The <thread/> element MAY possess a 'parent' attribute that
   identifies another thread of which the current thread is an offshoot
   or child.  The 'parent' attribute MUST conform to the syntax of the
   <thread/> element itself and its value MUST be different from the XML
   character data of the <thread/> element on which the 'parent'
   attribute is included.

      Implementation Note: The ability to specify both a parent thread
      and a child thread introduces the possibility of conflicts between
      thread identifiers for overlapping threads.  For example, one
      <thread/> element might contain XML character data of "foo" and a
      'parent' attribute whose value is "bar", a second <thread/>
      element might contain XML character data of "bar" and a 'parent'
      attribute whose value is "baz", and a third <thread/> element
      might contain XML character data of "baz" and a 'parent' attribute
      whose value is once again "foo".  It is up to the implementation
      how it will treat conflicts between such overlapping thread
      identifiers (e.g., whether it will "chain together" thread
      identifiers by showing "foo" as both a parent and grandchild of
      "baz" in a multi-level user interface, or whether it will show
      only one level of dependency at a time).

###

> In section 8.5.2.1, I encourage adding text explaining why delivering
> to all non-negative resources is SHOULD and not MUST. I understand
> from out-of-band communication that local policy may determine that
> some particular resource not receive the message, but the document
> does not say this. As written, I can see implementations that adopt a
> policy of delivering to the first non-negative resource and claim
> they've met the requirement.

I assume you're referring to this paragraph in the subsection about
message stanzas (8.5.2.1.1):

   o  If there is more than one resource with a non-negative presence
      priority then the server SHOULD either (a) deliver the message to
      the "most available" resource or resources (according to the
      server's implementation-specific algorithm, e.g., treating the
      resource or resources with the highest presence priority as "most
      available") or (b) deliver the message to all of the non-negative
      resources.

The traditional behavior was (a). Some implementations and deployments
follow (b) -- the first major example was the Google Talk service. In
the XMPP WG we had some discussions about this and decided to allow (b)
if the implementation or deployment deems that necessary or desirable.
However, making (b) a MUST and disallowing (a) was too large a change
from existing practice.

Peter

-- 
Peter Saint-Andre
https://stpeter.im/

---

From: Robert Sparks <[email-masked]>
Date: Thu, 20 Jan 2011 15:53:39 -0600

>      aggregate or for any given contact.
wfm
> to use <iq/> stanzas for in-band bytestreams for better flow control.

Noted. 

>   end recipients are provided in Section 8.4 of [XMPP-CORE].
wfm
> protocol-related peccadilloes (e.g., <bad-request/>).

Right now it's implicitly up to the implementation - consider making it explicit?
I can see interop arguments over this point.

> Given my reading of the community, SHOULD is as strong as we can get

My question was about "SHOULD check the syntax" not "SHOULD  treat it
as if". Apologies for not being more clear.
As you talk about the clarifications you indicate in 2), consider writing
your normative text more strongly and noting that there are existing implementations
that may not yet comply.

>   connected client as a cause for returning a stanza or stream error to

That text is more clear, but it still has the potential for the misread I'm pointing to. 
A change along the lines of "as a cause" to "as the sole reason" would close the 
hole I think I see.

>      only one level of dependency at a time).
That's certainly enough to get the implementer to start thinking about
the edge cases. I'm not sure it would cause them to recognize they need to deal with 
     <thread parent='foo'>
       foo
     </thread>
reasonably, and still wonder if the spec should say something about whether that should
be allowed, but leave that to your judgement.

>      available") or (b) deliver the message to all of the non-negative

For all of the similar paragraphs for "normal", "chat", and "headline"

> However, making (b) a MUST and disallowing (a) was too large a change

We might be talking past each other a bit (since I made a similar comment on 3920).
I'm not asking for discarding a and making b a MUST. I'm suggesting it would be
better to say MUST (a or b) instead of SHOULD (a or b).

That said, you could argue that the implementation choice I call about above could be
justified as an instance of a) with a local policy of "the first non-negative resource I can 
find is the 'most available'", so the difference may not be great enough to bother.

---

From: Peter Saint-Andre <[email-masked]>
Date: Thu, 20 Jan 2011 15:18:59 -0700

Snipping areas of agreement...

>> On 1/19/11 2:02 PM, Robert Sparks wrote:

<snip/>

> I can see interop arguments over this point.

Probably the best place for text on this point is rfc3920bis, because it
relates to generic error handling, not rosters specifically.

> your normative text more strongly and noting that there are existing implementations

I think that's a better path.

> A change along the lines of "as a cause" to "as the sole reason" would close the 

Done.

> be allowed, but leave that to your judgement.

The revised text I sent includes this sentence:

   The 'parent' attribute MUST conform to the syntax of the
   <thread/> element itself and its value MUST be different from the XML
   character data of the <thread/> element on which the 'parent'
   attribute is included.

Do you think we need to advise a client how to handle data that violates
the second clause?

> For all of the similar paragraphs for "normal", "chat", and "headline"

Right.

> better to say MUST (a or b) instead of SHOULD (a or b).

Yes, I think that would be appropriate.

> find is the 'most available'", so the difference may not be great enough to bother.

I think "MUST (a or b)" would be better.

Peter

-- 
Peter Saint-Andre
https://stpeter.im/

---

From: Robert Sparks <[email-masked]>
Date: Thu, 20 Jan 2011 16:28:14 -0600

Also trimming

On Jan 20, 2011, at 4:18 PM, Peter Saint-Andre wrote:

<snip/>

> Do you think we need to advise a client how to handle data that violates

No, I totally missed that on my read through (I focused on the Implementation note) - sorry.