Item1149: Validate META tags read from user-entered text

Priority: Enhancement
Current State: Closed
Released In: 1.1.0
Target Release: minor
Applies To: Engine
Component:
Branches:
Reported By: KennethLavrsen
Waiting For:
Last Change By: KennethLavrsen
This is a bug branched out from Item1119 which was partly closed in 1.0.2

This bug it to add further fixes for 1.1.0. Below all the text from 1119


I can use the raw editor to add bad meta e.g. %META:FILEATTACHMENT{"cockup"}% and get a crash when I try to save.

In fact I can crash it almost at will, just by finding unchecked params to not add to META:FILEATTACHMENT.

It's not good that malformed meta-data can crash Foswiki, but that should be a manageable problem as meta-data should only be generated by code. What is much scarier is that a user can type meta-data into their topics and it gets blindly incorporated. This has been the case for a long, long time, but that doesn't excuse it.

Having slept on it I think this is a killer. Marking it as Urgent.

I think the problem is just in the META parser - it should only accept meta-data at the beginning and end of the topic, and it should only accept well-formed meta-data. However that still makes topics to user-added %META: statements at the start and end of the topic text. Maybe that's a good thing; can it be sold as a feature? To fix it properly would involve escaping the %META: string at the start of a line, which would have knock-on effects all over the place.

-- CrawfordCurrie - 22 Feb 2009

The key question is if any security boundaries are breached -- can users add anything manually that they could not add through other means? (From a purist point of view, I'd prefer the proper fix, but I understand that it may not be feasible at this time.)

-- IsaacLin - 23 Feb 2009
I thought this through a bit further last night. The ability to add META in any position is very powerful, but as you say, what is the impact? Can a footpad mug your Foswiki by adding META to your topics?

Default META has the following types: FILEATTACHMENT, TOPICPARENT, TOPICINFO, TOPICMOVED, FORM, FIELD, and PREFERENCE. Any or all of these might be used to "mislead" a reader of the topic, but since to save meta you have to have write access, they can all be used that way anyway, if you know what you are doing. So I don't think there are any security implications here. Extensions may add other meta types - for example, META:WORKFLOW is added by the WorkflowPlugin. So there can be no centralised concept of what constitutes "correct" meta. Also, the fact that meta is embeddable in topic text is powerful and is used by several extensions. So the only risk is that malformed meta can result in a crash.

The best approach would be to support a macros specification that could be used to check META macros; similar in concept to XSL. For example, we could specify META:FILEATTACHMENT as:
'META:FILEATTACHMENT' => {
    name => { type => 'string', required => 1 },
    version => { type => 'string' },
    path => { type => 'string' },
    size => { type => 'integer' },
    date => { type => 'integer' },
    user => { type => 'loginname' },
    comment => { type => 'string' },
    attr => { stype => 'string' },
    movedfrom => { type => 'string' },
    movedby => { type => 'string' },
    movedto => { type => 'string' },
    moveddate => { type => 'integer' }
}
This spec would be used to check META:FILEATTACHMENT. Anything that doesn't match the spec will be rejected.

For non-standard META we could simply accept anything that wasn't in the above table; extensions could register new META types using something like:
Foswiki::Func::registerMETA('META:WORKFLOW' => { ... spec as shown above ... });
Note that there is potential in this spec to be even smarter and add validator functions to the spec. Also, this same type of spec can be used to specify other types of macro, such as those added using registerTagHandler.

In the short term we can simply ensure that any invalid META can't crash Foswiki; though there is still a risk of malformed meta breaking an extension.

-- CrawfordCurrie - 24 Feb 2009

while I always support the registration of 'valid' syntax, imo it should not be possible or valid for a user to enter a META string that then is interpreted by foswiki as actual META.

whether this means that there needs to be a separator between text & the trailing META (even a single line feed), or that entered META is escaped i don't know, but the fact that anyone can add a string that is valid as per the above spec, but points to a hidden or non-existent attachment (or workflow or whatever) seems to me to not be desirable.

basically, registration does not help the root cause, just stops the symptom.

while in T* it was violently argued that it was desirable to allow META to be aded by hand, i hope we can remove this backdoor form Foswiki. Its only used by people that should know better, and generally are capable of writing code to do it properly.

-- SvenDowideit - 24 Feb 2009

Do you have links to that debate? I would like to understand the reasoning.

-- CrawfordCurrie - 24 Feb 2009

Some thoughts:

Violence is hardly an indicator of correctness. I agree that this is undesirable, as it can result in accidental bad behavior as well as some vulnerabilities to malicious exploits. See Tasks.Item773 for an accident.

People who can't write code CAN use the topics more actions editor - it currently handles preferences, but could be extended for the other valid edits. Some that I can think of: removing a "moved" marker -- valid when you develop in sandbox and move to production, or after some time. Renaming a form field. Others?

This is a rather large, ugly problem. It probably needs to be solved incrementally. Ideally, metadata would be in a separate container - not merged with the topic text. But finding all the places where code does a search for ^%META: - including extensions - instead of using the API functions is likely to be slow and error-prone. Until someone takes that on, I agree that markers at the beginning and end of a topic would be an improvement. But the end is tricky - one needs to prevent the end marker from being inserted by someone editing the text. E.G. consider

Meta stuff
--delim
topic
--delim <-added by user
bad meta
--delim <-real delim
good meta

Yes, one could parse the topic backwards for the last delimiter -- but then one has to be sure that every topic HAS a delimiter. Otherwise, you can use the editor to add bad metadata to a topic that has none.

The obvious alternative is to delimit topic text - which has similar issues.

Since all of these options seem to need some migration - why not put all metadata at the start of a topic? Every valid topic has some, and we can simply refuse to recognize metadata after an end marker.

As for migration - while it's tempting to incrementally fix topics as they're saved, I think a (nominally) one-time migration script is probably a better approach. (Note that twiki->foswiki script would have to do this.) Less likely to have bugs and an on-going maintenance headache. Of course, if there are extensions that don't use the META api, they may break topics. In this case, their metadata will appear as topic text - so it can be fixed fairly quickly. Re-running the migration script could be a work-around. At least the ugliness stays in one place.

All code should be defensive - but there will be escapes. As things stand, we can't even assign blame to corrupt metadata - there's always the excuse that 'the user did it'.

I like the idea of validating metadata. Perhaps it should be added to a file at installation rather than dynamically at runtime as suggested? That would allow pre-compilation (someday), enable conflict checking prior to installing an extension, and allow a maintenance script to easily check all topics for corruption. The drawback is that one would have to be able to de-register syntax.

For extensions that are in the Foswiki svn, we could have the checkin script reject anything that includes %META (the API files can code so as not to match)... And maybe provide the check as a script for anyone who doesn't checkin to apply to their legacy code.

-- TimotheLitt - 24 Feb 2009

Nice and valid discussion.

But let us wrap up.

  • It is urgent in 1.0.2 context (or 1.1.0) to fix the crashing.
  • It would be nice if META in topic text is not resulting in real meta to be saved. How to do that need careful thinking and is for sure 1.1.0 or later material.

-- KennethLavrsen - 24 Feb 2009

I checked in protection for the crash above. I want to keep this report as Urgent, but it can now be deferred to 1.1. Kenneth closed the bug 1119 and opened this instead

-- CrawfordCurrie - 25 Feb 2009

I changed the headline of this report to reflect the discussion. It was previously "Prevent text with META tags from being saved as real meta (scope 1.1)"

-- CrawfordCurrie - 06 Aug 2009

Amusingly enough, this fix showed up a lurking problem in PatternSkin.

-- CrawfordCurrie - 06 Aug 2009

Did some further re-engineering an added a unit test.

-- CrawfordCurrie - 12 Aug 2009

ItemTemplate edit

Summary Validate META tags read from user-entered text
ReportedBy KennethLavrsen
Codebase 1.0.1, trunk
SVN Range Foswiki-1.0.0, Thu, 08 Jan 2009, build 1878
AppliesTo Engine
Component
Priority Enhancement
CurrentState Closed
WaitingFor
Checkins distro:a4f1600ed267 distro:8617b5cd19a4 distro:4254183a3bcb
TargetRelease minor
ReleasedIn 1.1.0
Topic revision: r12 - 04 Oct 2010, KennethLavrsen
 
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. see CopyrightStatement. Creative Commons License