search

Consensys / truffle-security

MythX smart contract security verification plugin for Truffle Framework
https://mythx.io
124 stars 28 forks source link

Unable to retrieve correct line numbers from corresponding srcMaps #102

Closed shashank-srikant closed 5 years ago

shashank-srikant commented 5 years ago

Summary

I'm trying to retrieve line numbers from the srcMaps which mythril-classic provides, as well srcMaps pertaining to some select nodes in a contract's AST. In doing so, in some cases,

Details

Steps to replicate

Functions which I call to retrieve line numbers are -

I call byteOffset2lineColumn() on the following parameters - [Solidity source] The source code that I am analyzing is https://etherscan.io/address/0xf6da07a8ffb5683314715d8c7a1a5c952b4e74ca#code

[Contract] I am specifically analyzing the contract StandardToken

[AST dump] solc generated AST for the solidity source. https://gist.github.com/shashank-srikant/9e972a692dd480c12ca2b588d170f2eb

[Bytecode dump] solc generated bytecode for the solidity source. https://gist.github.com/shashank-srikant/d77c0e5bfc58c51155a7d5dddeeec673

[Mythril generated error report] https://gist.github.com/shashank-srikant/aa2164f5e01e6cf66697059413a7e3a3 I parse all the srcMaps from here and pass it to the above functions one after another.

Library versions

Observed output

The following line numbers are output for the three srcMaps in the Mythril error for this contract - input: 2456:1:0 output [ {line:77, column:103}, {line:77, column:104} ]

input: 2432:1:0 output [ {line:77, column:79}, {line:77, column:80} ]

input: 2419:1:0 output [ {line:-1, column:0}, {} ]

Expected output

Similarly, on picking up any srcMap corresponding to an arbitrary node in the AST of the contract StandardToken also produces the output [ {line:-1, column:0}, {} ]

Suggestions on what's going on?

rocky commented 5 years ago

Thanks for the detailed report. I will look at this when I get a chance.

I note also that this isn't strictly related to truffle security or its functioning, but rather you would like to use truffle-security more as a library.

Extracting code from truffle-security and turning that extraction into a proper library is one of the things that we will be doing over the next couple of months. And, no doubt, bugs or features arising from the situation mentioned above will be the kind of thing we want to make sure is addressed.

Right now though, things are extremely busy and we are trying to get out a major release. I hope to look at this after the upcoming release.

rocky commented 5 years ago

There is a lot of detail here, but a a lot of work to try to put it together to make it make sense. And on the other hand, there is some missing information.

I have already spent more trying out how to piecing together fragments of information than the time I alloted to it this week. (Pro tip: formatted json is much nicer to look at than one line of JSON that your gists have.)

Reporting a bug really isn't that hard. As rms wrote a while back you just give complete input and complete output. and include any custom data or programs associated with that. The main goal is to make easy for someone else to easily reproduce the problem.

Also give the smallest example that shows a problem. Yes that often takes a bit of work to get, but don't worry, I can wait fhe additional time that is needed to narrow the problem. Often when you narrow the problem in such a way you'll also gain insight as to what might be going wrong and thus how to avoid the problem.

The bug-reporting process described above adapted here is:

  1. Create a truffle project with the contract in it with a valid truffle security setup.
  2. Run truffle run verify to make sure everything works.
  3. create in the truffle project some simple standalone code that you can run that exhibits the problem
  4. run the program capturing in a file both the complete input comand and the complete output, (not those bits and pieces that you think are relevant - remember you are asking for help which suggest you might not know which is relevant - so give everything).
  5. If you run other commands to get information like Mythril classic, ok include a log file containing file the both complete input command you ran, and the complete output you got.
  6. Commit all of this in your github project.
  7. Send me a link to the github project which also include all logs from running commands.

From the looking at the bits and pieces, I do have some thoughts about what is going on and how to fix. However guessing isn't my thing: I would go into the stock market if that were. So best to wait, see what the data says and then follow where that leads.

shashank-srikant commented 5 years ago

Thanks. Have added the file i use to call the byteOffset2lineColumn() in truffle-security/lib/issues2eslint.js

https://github.com/shashank-srikant/mythril-line-numbers

this isn't a truffle project; just a javascript being invoked through node. this is how i've been trying to access byteOffset2lineColumn() till now. running this file reproduces the issue i have described above. let me know if this suffices.

thanks.

rocky commented 5 years ago

Much better, but alas, still could use more. Specially code that creates that data using truffle-security.

Bugs/Issues here are for truffle-security so if you want to report a bug here, you should start off with a valid truffle project. (That makes it easy for us to work on and fix things.)

Adding additional code as suggested and you've done is helpful, but not sufficient.

This might not be a problem with the code, but a problem in MythX or somewhere else.

shashank-srikant commented 5 years ago

Specially code that creates that data using truffle-security.

i did not use any truffle-security code to generate the data.

byteOffset2lineColumn( ) is the entry point in my workflow. i generate data using the solc and myth commands i have listed in the repo's readme. i then invoke this truffle-security library in the manner i've described in the script on my repo.

if this is not sufficient for it to be raised as a truffle-security issue, could you suggest where i could raise this issue? because i do not intend to setup truffle and run all of this via truffle to just pull out line numbers for the errors i see in my current workflow.

thanks.

shashank-srikant commented 5 years ago

meanwhile, let me try recreating this through truffle.

rocky commented 5 years ago

if this is not sufficient for it to be raised as a truffle-security issue, could you suggest where i could raise this issue? because i do not intend to setup truffle and run all of this via truffle to just pull out line numbers for the errors i see in my current workflow.

Right now, sadly, there is no place. Over the next couple of months we will be working on a javascript library (or SDK) that includes all of the important parts here and from other projects.

rocky commented 5 years ago

Hmm... let me understand something. Is the situation that by some means other than MythX you are getting a bytecode offset and are expecting to get a source location from that, whatever the bytecode offset (assuming it starts at a valid instruction boundary)?

If that's the case there will undoubtably be places in bytecode offsets that aren't associated with any source code.

For example if you are working with deployed bytecode, there is some boilerplate code to load the contract. Because it is not clear exactly where in the source code that should come from, solc uses -1 to indicate that there is no source code location.

So from your standpoint, whenever you see -1, you should just assume you are not going to be able to get source code.

In the context of MythX however, whenever that reports a vulnerability, then we will use magical means (some of which I haven't invented yet), to turn the -1 into something useful.

shashank-srikant commented 5 years ago

let me understand something. Is the situation that by some means other than MythX you are getting a bytecode offset and are expecting to get a source location from that, whatever the bytecode offset (assuming it starts at a valid instruction boundary)?

i currently have two use-cases,

rocky commented 5 years ago

MythX and Mythril classic are related but different.

Let's break down each of your two cases. What to do about them is different.

  • for the bytecode offset returned by mythril-classic (which presumably is the same as MythX? not sure.), i want to pull out corresponding source locations.

So again, give the full mytrhil classic command you typed and the full output you got.

  • the other use-case is i want line numbers corresponding to the srcMaps/bytecode offsets which solc provides as part of a contract's AST object. i find a -1 being returned even for such offsets.

I think you're going to have to live with -1. I suppose you could try opening an issue with solc, but I suspect what I wrote above is going to be the situation.

shashank-srikant commented 5 years ago

So again, give the full mytrhil classic command you typed and the full output you got.

input: (mentioned in https://github.com/shashank-srikant/mythril-line-numbers/blob/master/README.md) myth -xo jsonv2 data/{filename}.sol:{contractname} > data/{filename}.sol:{contractname}.json where filename: sol_0xf6da07a8ffb5683314715d8c7a1a5c952b4e74ca contractname: StandardToken

output: https://github.com/shashank-srikant/mythril-line-numbers/blob/master/data/sol_0xf6da07a8ffb5683314715d8c7a1a5c952b4e74ca.sol:StandardToken.json

[  
   {  
      "issues":[  
         {  
            "description":{  
               "head":"A reachable exception has been detected.",
               "tail":"It is possible to trigger an exception (opcode 0xfe). Exceptions can be caused by type errors, division by zero, out-of-bounds array access, or assert violations. Note that explicit `assert()` should only be used to check invariants. Use `require()` for regular input checking."
            },
            "extra":{  

            },
            "locations":[  
               {  
                  "sourceMap":"2456:1:0"
               }
            ],
            "severity":"Low",
            "swcID":"SWC-110",
            "swcTitle":"Assert Violation"
         },
         {  
            "description":{  
               "head":"A reachable exception has been detected.",
               "tail":"It is possible to trigger an exception (opcode 0xfe). Exceptions can be caused by type errors, division by zero, out-of-bounds array access, or assert violations. Note that explicit `assert()` should only be used to check invariants. Use `require()` for regular input checking."
            },
            "extra":{  

            },
            "locations":[  
               {  
                  "sourceMap":"2432:1:0"
               }
            ],
            "severity":"Low",
            "swcID":"SWC-110",
            "swcTitle":"Assert Violation"
         },
         {  
            "description":{  
               "head":"The binary addition can overflow.",
               "tail":"The operands of the addition operation are not sufficiently constrained. The addition could therefore result in an integer overflow. Prevent the overflow by checking inputs or ensure sure that the overflow is caught by an assertion."
            },
            "extra":{  

            },
            "locations":[  
               {  
                  "sourceMap":"2419:1:0"
               }
            ],
            "severity":"High",
            "swcID":"SWC-101",
            "swcTitle":"Integer Overflow and Underflow"
         }
      ],
      "meta":{  

      },
      "sourceFormat":"evm-byzantium-bytecode",
      "sourceList":[  
         "0x78cceee761f0c8065c1bc2a4006d669abf2b4190bc6c80ec3ad06d5dd5034d5c"
      ],
      "sourceType":"raw-bytecode",
      "time_taken":143.149
   }
]
rocky commented 5 years ago

For what it's worth, running truffle-security gives:

/home/rocky/truffle/DoubleLandICO/contracts/DoubleLandICO.sol
    1:0   warning  A floating pragma is set                                SWC-103
    5:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
    7:23  warning  integer overflow                                        SWC-101
   11:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
   11:23  warning  Local variable shadows a state variable                 SWC-119
   31:8   warning  A reachable exception has been detected                 SWC-110
   36:20  error    The binary addition can overflow                        SWC-101
   47:32  warning  The state variable visibility is not set                SWC-108
   66:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
   81:54  warning  The state variable visibility is not set                SWC-108
  126:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  197:9   warning  The state variable visibility is not set                SWC-108
  279:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  303:4   warning  State variable shadows another state variable           SWC-119
  330:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  349:4   warning  State variable shadows another state variable           SWC-119
  435:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  442:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  446:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  499:17  warning  assertion violation                                     SWC-110
  518:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  526:4   warning  Use of disallowed state mutability modifier "constant"  SWC-111
  527:15  warning  The contract executes an external message call          SWC-107

So that suggests that there's something in your code that isn't quite right. We are not set up for assistence with how to develop your programs using bits and pieces of the code here.

Perhaps when we have a library or SDK maybe in a couple of months we will be in a better position to handle the kind of thing you are trying to do.

In the meantime I hope you can find what you are doing wrong possibly by more closely following what we are doing here or by using MythX instead of Mythril Classic.

shashank-srikant commented 5 years ago

i notice that this is analyzing the entire file, and not just the specific contract StandardToken? the results i shared above pertain to Mythril-classic's analysis of just standardToken.

could you please share the repository/truffle project you created and ran? i got this error when trying to run truffle-security on the specific contract standardToken: https://github.com/ConsenSys/mythx-developer-support/issues/16

thanks.