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

FeatureRequiresRootPriviledgeOnUnixRule for Gendarme #59

Closed
GoogleCodeExporter opened this issue Jun 3, 2015 · 13 comments
Closed

FeatureRequiresRootPriviledgeOnUnixRule for Gendarme #59

GoogleCodeExporter opened this issue Jun 3, 2015 · 13 comments

Comments

@GoogleCodeExporter
Copy link

This task is to create a new portability rule for Gendarme [1]. This rule
must detect two common patterns that Windows developers may not be aware.

1) Setting Process.PriorityClass to something else than
ProcessPriorityClass.Normal won't work for normal users

2) Using the System.Net.NetworkInformation.Ping class won't work for normal
users

To complete the task it must include:
- the rule source code;
- unit tests to ensure the rule works correctly;
- documentation for the web site (see [2] for an example)

This tasks should take between 2-5 days (depending if you already know
Gendarme/Cecil or not).

Note: There are other similar cases (where root is needed) and this rule
will be extended in the future to track similar patterns (but this is
outside this task).

[1] http://www.mono-project.com/Gendarme
[2] http://groups.google.com/group/gendarme

Original issue reported on code.google.com by sebastie...@gmail.com on 21 Dec 2007 at 4:55

@GoogleCodeExporter
Copy link
Author

Original comment by sebastie...@gmail.com on 21 Dec 2007 at 6:11

  • Changed state: Open

@GoogleCodeExporter
Copy link
Author

I claim this task.

Original comment by andreas....@gmail.com on 25 Dec 2007 at 12:47

@GoogleCodeExporter
Copy link
Author

Original comment by jpo...@gmail.com on 25 Dec 2007 at 1:55

  • Changed state: Claimed
  • Added labels: ClaimedBy-andreas.noever

@GoogleCodeExporter
Copy link
Author

I have tested some approaches to the problem and settled for a method blacklist
(MethodNotSupportedRule ). The rule checks each call/calli/callvirt/newobj 
against a
blacklist (Ping.ctor, Process.set_PriorityClass).

If the code is ok, I'll start on the documentation.

Original comment by andreas....@gmail.com on 26 Dec 2007 at 4:59

Attachments:

@GoogleCodeExporter
Copy link
Author

Nice code :) but this is no quite what's needed (at least in this case). What 
you did
is very similar to what MoMA checks for (and could prove useful too).

* While the name of the rule match what your doing, it doesn't match the task
description. I.e. in both case the methods *are* supported on Mono, but only if 
the
user is root (or has equivalent privileges). Please use the
FeatureRequiresRootPrivilegeOnUnixRule name (this will be a Portability rule).

* Both the Process and Ping classes are not sealed. So the rule wouldn't work 
on:
  public class MyPing : Ping {}
  public class MyProcess : Process {}
  Note: please add both cases to the unit tests.

* IIRC setting PriorityClass to Normal is ok for all users. Setting it to any 
other
value requires root privileges. So you need to check the value assigned to the
property and warn if it's not Normal. Note: this is not a 100% safe solution 
(e.g. if
a variable, not a constant, is assigned) but it will reduce false-positives.


* Small fix: Please change the old pattern

    if ((method.Body == null) || (method.Body.Instructions == null))
        return null;

  to the newer one

    if (method.HasBody)
        return runner.RuleSuccess;

  as this will become more important in future version of Gendarme (where an enum
will be returned).

* Small fix: For performance reason (rules can be executed thousands of times) 
avoid
creating MessageCollection unless you really need it. Several rules have been 
updated
to do this recently.

Thanks!

Original comment by sebastie...@gmail.com on 26 Dec 2007 at 5:30

@GoogleCodeExporter
Copy link
Author

* The Ping part is now covert by a ModuleRule. It checks the TypeReferences for 
a
reference to the Ping type.

* Although Process is not sealed, set_PriorityClass is (methods are sealed by
default). The call opcode references Process.set_PriorityClass, even if it is 
invoked
on a subclass. I have added a unit test to confirm this.

* The rule now only warns if a value != ProcessPriorityClass.Normal is used. 
(very
primitive check). But since ProcessPriorityClass.Normal is the default (?)
false-positives should be rare anyways.

* Fixed the skeleton code; added lazy MessageCollection creation; (I took the
original code from the NewLineLiteralRule (Portability). This rule still uses 
the old
pattern.)

Original comment by andreas....@gmail.com on 26 Dec 2007 at 8:07

Attachments:

@GoogleCodeExporter
Copy link
Author

Wow, your IModuleRule is a really nice trick :) and is actually a good match for
something (a different model) we discussed for executing rules at the last mono
summit. I'm impressed :)

However it lacks precision, since you cannot pinpoint which method(s) are using 
the
Ping class.

I suggest that you 
* keep the module rule to set a boolean flag (for a ping reference) and return
success at this level;
* add an additional check in the IMethodRule.CheckMethod part (if the ping flag 
is
set) to see if the method create or use Ping

Note: I'll check the Process code later tonight.

Original comment by sebastie...@gmail.com on 26 Dec 2007 at 10:03

@GoogleCodeExporter
Copy link
Author

Thanks :)

But my current version works without the TypeReferences. The constructor of 
MyPing
calls Ping..ctor :)

Current status:

new Ping() <- warning
class MyPing : Ping {} <- warning
new MyPing() <- no warning

process.PriorityClass = PriorityClass.Normal <- no warning
process.PriorityClass = PriorityClass.AboveNormal etc <- warning
myProcess.PriorityClass = PriorityClass.AboveNormal <- warning

Original comment by andreas....@gmail.com on 26 Dec 2007 at 11:53

Attachments:

@GoogleCodeExporter
Copy link
Author

Looks great :) but there's one case that I think should be handled as well ;-)
Here's the test case...

public void UsePing (Ping ping)
{
    // e.g. Ping could be supplied from another assembly
    // but Gendarme should still flag it's usage inside the analyzed assembly
    ping.Send ("127.0.0.1");
}

[Test]
public void TestUsePing ()
{
    // use an already created Ping instance
    MethodDefinition method = GetTest ("UsePing");
    Assert.IsNotNull (rule.CheckMethod (method, new MinimalRunner ()));
}

p.s. sorry I missed you earlier on IRC :(

Original comment by sebastie...@gmail.com on 27 Dec 2007 at 1:36

@GoogleCodeExporter
Copy link
Author

Original comment by andreas....@gmail.com on 27 Dec 2007 at 2:13

Attachments:

@GoogleCodeExporter
Copy link
Author

Gendarme.Rules.Portability

FeatureRequiresRootPrivilegeOnUnixRule

This will check for usage of restricted features.
* System.Net.NetworkInformation.Ping: This type can only be used by root on unix
systems. You can execute the ping command and parse it's result as an 
alternative.

* System.Diagnostics.Process: The PriorityClass property can only be set to 
Normal by
non root users. Do a platform check before assigning it:
if ( Environment.OSVersion.Platform != PlatformID.Unix )
   process.PriorityClass = ProcessPriorityClass.AboveNormal;



Original comment by andreas....@gmail.com on 27 Dec 2007 at 2:45

@GoogleCodeExporter
Copy link
Author

Great job!

Original comment by sebastie...@gmail.com on 27 Dec 2007 at 2:47

  • Changed state: Closed

@GoogleCodeExporter
Copy link
Author

Committed to SVN r91911/2.

Original comment by sebastie...@gmail.com on 27 Dec 2007 at 3:56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant